-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
.pr_agent_accepted_suggestions
| PR 15908 (2026-06-05) |
[correctness] Wrong setAll update order
Wrong setAll update order
ImporterPreferences#setAll updates apiKeys before persistCustomKeys, but JabRefCliPreferences stores/clears OS keyring entries on apiKeys changes based on the current persist flag, so reset/import can leave stale keys in the keyring or store keys when persistence is being turned off. This can break the expected semantics of Preferences reset/import for custom API keys.ImporterPreferences#setAll currently calls setApiKeys(...) before updating persistCustomKeys. Because JabRefCliPreferences attaches an InvalidationListener to ImporterPreferences#getApiKeys(), modifying the set triggers storeFetcherKeys(...), which decides whether to write/delete keyring entries based on shouldPersistCustomKeys(). With the current order, storeFetcherKeys(...) can run using the previous persist flag value during preference reset/import.
-
clear()andimportPreferences()callgetImporterPreferences().setAll(...). -
getApiKeys()changes trigger keyring store/clear.
- jablib/src/main/java/org/jabref/logic/importer/ImporterPreferences.java[118-130]
Suggested fix: In
setAll, setpersistCustomKeysbefore callingsetApiKeys(...)(matching the ordering already documented/used inWebSearchTabViewModel.storeSettings).
[correctness] Custom importers not loaded
Custom importers not loaded
JabRefCliPreferences#getCustomImportFormats always returns defaults because it checks for the exact key "customImportFormat", but the data is stored under numbered keys ("customImportFormat0", "customImportFormat1", …). This causes saved custom importers to be ignored after restart/import and effectively loses user configuration.getCustomImportFormats checks hasKey(IMPORTER_CUSTOM_FORMAT) (i.e., customImportFormat) and returns defaults if absent. However, the series is stored under customImportFormat0, customImportFormat1, ... so the guard prevents loading custom importers.
-
hasKeychecks only an exact key in the backing store. -
getSeries(prefix)readsprefix + N. -
storeCustomImportFormatswritesprefix + i.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2132-2154]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2157-2162]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[589-591]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[781-789]
Replace the guard with a check that matches the storage format, e.g.:
-
if (!hasKey(IMPORTER_CUSTOM_FORMAT + "0")) return defaults;or - remove the
hasKeyguard and instead do: List<String> series = getSeries(IMPORTER_CUSTOM_FORMAT);if (series.isEmpty()) return defaults;- parse
series.
[correctness] Catalog defaults not applied
Catalog defaults not applied
JabRefCliPreferences#getImporterPreferencesFromBackingStore ignores the provided defaults for catalogs by calling getStringList(IMPORTER_CATALOGS) without falling back to defaults.getCatalogs() when the preference is absent/blank. On fresh profiles or incomplete preference imports, importer catalogs become empty, disabling web search defaults in the GUI (JabRefGuiPreferences extends JabRefCliPreferences).getImporterPreferencesFromBackingStore(ImporterPreferences defaults) currently loads catalogs using getStringList(IMPORTER_CATALOGS) without applying the passed-in defaults when the key is missing/blank, resulting in an empty catalog list on first run or when imported preferences omit the key.
-
getImporterPreferences()passesImporterPreferences.getDefault()intogetImporterPreferencesFromBackingStore(...). -
getStringList(key)usesget(key)internally; if the key is absent, it can yield an empty list. - JabRef GUI preferences inherit this behavior because
JabRefGuiPreferences extends JabRefCliPreferences.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2095-2130]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[593-595]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[675-690]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[78-79]
In getImporterPreferencesFromBackingStore:
- Compute catalogs with a fallback:
List<String> catalogs = getStringList(IMPORTER_CATALOGS);if (catalogs.isEmpty()) { catalogs = defaults.getCatalogs(); }- pass
catalogsinto theImporterPreferencesconstructor. Alternative: introduce a helpergetStringList(key, List<String> defaultList)that usesget(key, convertListToString(defaultList)).
| PR 15896 (2026-06-03) |
[reliability] `inputs.gradleCommand` undefined in action
`inputs.gradleCommand` undefined in action
The composite GitHub Action `.github/actions/package` no longer defines the `gradleCommand` input but still references `inputs.gradleCommand` in the Smoke test (and ShadowJar) steps, which can result in an empty/invalid command and deterministic workflow failures. This undermines consistent, configured Gradle invocation in CI.The composite action .github/actions/package/action.yml removed the gradleCommand input, but some steps still reference ${{ inputs.gradleCommand }}, which can evaluate to an empty string and produce an invalid Gradle command, breaking CI (including the binaries workflow).
The gradleCommand input was removed to standardize on a single Gradle invocation, and .github/workflows/binaries.yml no longer passes gradleCommand into the action. The action already hardcodes a Gradle invocation for the jpackage step, so the remaining ${{ inputs.gradleCommand }} usages should be updated to match that standardized approach (or the input should be reintroduced if standardization is not desired).
- .github/actions/package/action.yml[4-40]
- .github/actions/package/action.yml[71-82]
- .github/actions/package/action.yml[129-139]
| PR 15895 (2026-06-03) |
[correctness] Staged/untracked changes ignored
Staged/untracked changes ignored
The new check-formatting composite action is documented as failing on “uncommitted changes”, but the underlying script treats an empty `git diff` (unstaged only) as a clean tree. This means staged changes or untracked files can exist without failing CI, undermining the intent of the formatting/OpenRewrite gate.check-formatting relies on .github/format-diff-annotations.sh, which currently uses plain git diff and exits 0 when that output is empty. This only detects unstaged modifications, so staged changes and untracked files can still be present while CI reports “clean”.
The action’s description and usage implies it gates on “uncommitted changes”, so the check should cover at least:
- unstaged changes
- staged changes
- (optionally) untracked files
- .github/format-diff-annotations.sh[21-26]
- .github/actions/check-formatting/action.yml[34-43]
- .github/actions/check-formatting/action.yml[1-5]
[maintainability] Misleading annotation comment
Misleading annotation comment
The script header comment still says the message appends only `lines -`, but flush() now emits `line N` for single-line hunks. This mismatch can confuse future maintenance/debugging of the annotation output format.The header comment describing the appended range format is now inaccurate after changing the output to use line N for single-line hunks.
Keeping this comment correct matters because it documents the externally-visible annotation message format that developers see in CI.
- .github/format-diff-annotations.sh[9-14]
- .github/format-diff-annotations.sh[64-75]
| PR 15880 (2026-06-02) |
[correctness] Wrong file on deletions
Wrong file on deletions
The script only updates `file` on `+++ b/...`, so diffs that use `+++ /dev/null` (file deletions) will keep the previous file value and emit annotations against the wrong path. This can make CI annotations misleading/incorrect, especially in the OpenRewrite job that also uses this script..github/format-diff-annotations.sh sets file only when matching +++ b/.... For deletion diffs (+++ /dev/null), file is never updated, so subsequent ::error file=... annotations can be emitted for the previous file.
This script is used in CI to annotate formatting and OpenRewrite diffs. OpenRewrite can produce diffs including file deletions, so +++ /dev/null is a realistic case to support.
- Update parsing to reliably set
filefor every file-level diff, including deletions (e.g., parsediff --git a/... b/...and/or handle+++ /dev/null). - file/path focus:
- .github/format-diff-annotations.sh[61-64]
[correctness] Add-only hunks mis-anchored
Add-only hunks mis-anchored
The new hunk walker tracks only old-side line numbers and no longer parses `+newStart,+newCount`, so hunks with `oldCount=0` (pure insertions / new-file-style hunks) collapse to a single old-side line (or line 1 when oldStart=0). This breaks the “annotate only changed lines” behavior for insert-only changes by pointing at an incorrect range.The script only parses the old-side start ($2) and maintains only an oldLine counter. For addition-only hunks (@@ -X,0 +Y,N @@), annotations cannot be anchored to the actual changed new-side lines and instead collapse to a single old-side line number.
CI git diff output can include hunks where oldCount=0 (pure insertions). In those cases, the annotation should use the new-side range (newStart..newStart+N-1) or otherwise provide a meaningful range that corresponds to committed code developers can edit.
- Parse both sides from the hunk header (
-oldStart,oldCountand+newStart,newCount). - Maintain both
oldLineandnewLinecounters while scanning hunk body. - When a run contains no old-side consumption (or
oldCount==0), anchor the annotation on the new-side range. - file/path focus:
- .github/format-diff-annotations.sh[65-82]
[correctness] Quoted paths not parsed
Quoted paths not parsed
The awk parser only matches unquoted `--- a/...` and `+++ b/...` headers, so when `git diff` emits quoted headers (e.g., `--- "a/path with spaces"`) the script may keep a stale `file` value (or empty) and emit `::error` annotations against the wrong path. This makes CI annotations misleading/unactionable for changed files whose names require quoting (spaces/special characters)..github/format-diff-annotations.sh extracts the file path only from unquoted diff headers (--- a/... and +++ b/...). When git diff quotes paths (commonly for filenames containing spaces), those regexes don’t match, so fileRaw/file won’t be updated and subsequent annotations can point at the previous file or have an empty file=.
Concrete example of headers that won’t match today:
--- "a/config/Eclipse Code Style.epf"+++ "b/config/Eclipse Code Style.epf"
This repository contains tracked files with spaces in their path, so this is not purely theoretical.
- .github/format-diff-annotations.sh[64-71]
- Add additional header matchers for quoted paths, e.g.:
-
^--- "a/… strip leading--- "a/and trailing" -
^\+\+\+ "b/… strip leading+++ "b/and trailing"(Optionally handle backslash-escaped quotes within the name if you want to be extra robust.)
- Consider resetting
fileRaw/fileon^diff --gitto avoid accidentally reusing the previous file if a header isn’t recognized. - Optionally, make
flush()a no-op iffileis empty to avoid emitting invalid annotations.
| PR 15873 (2026-06-01) |
[security] Whitespace collapses PR number
Whitespace collapses PR number
The `Read pr_number.txt` step deletes *all* whitespace from the artifact (`tr -d '[:space:]'`), so malformed content like `12 3` becomes `123` and passes the numeric regex check. This can cause the workflow to target/comment on the wrong PR while still treating the artifact as valid.The workflow currently uses tr -d '[:space:]' to remove all whitespace from pr_number.txt before validating it. This can change the PR number by concatenating digits across spaces/tabs/newlines, allowing malformed content to become a different numeric value and still pass validation.
The artifact is produced as a single-line PR number (with a trailing newline from echo). The validation should therefore accept only a single line containing digits (optionally ending with \r\n), and reject any additional characters/lines/whitespace rather than deleting them.
- .github/workflows/pr-comment.yml[64-80]
- Read exactly the first line (
IFS= read -r PR_NUMBER < "$ARTIFACT_FILE"). - Strip only a trailing carriage return (
PR_NUMBER="${PR_NUMBER%$'\r'}"). - Reject if the file has more than one line (e.g.,
tail -n +2 "$ARTIFACT_FILE" | grep -q .→ invalid). - Validate with a strict digits-only check (e.g.,
[[ "$PR_NUMBER" =~ ^[0-9]+$ ]]). - Do not remove internal whitespace (no
tr -d '[:space:]').
| PR 15871 (2026-06-01) |
[correctness] Unescaped annotation properties
Unescaped annotation properties
The script emits `::error file=...,line=...,endLine=...` using raw `file` values from `git diff`, so paths containing characters that must be escaped for GitHub Actions workflow-command properties (e.g., `:`, `,`, `%`, newline) can produce malformed annotations or wrong parsing. The repo already contains a dedicated escape utility for exactly this annotation format, so this script is inconsistent with established behavior and can silently drop annotations in edge cases..github/format-diff-annotations.sh prints GitHub Actions workflow commands (::error ...) but does not escape the file property (and generally any property/data field). GitHub Actions requires percent-encoding for special characters in both the message data and in key=value properties; otherwise annotations can be parsed incorrectly or ignored.
The codebase already has an established escaping implementation (GitHubActionsEscape.property/data). The bash script should apply the same escaping rules when printing file=... (and any other dynamic property/data fields).
Existing Java-based annotation writers escape both properties and data explicitly, including : and , for properties.
- .github/format-diff-annotations.sh[19-30]
- jablib/src/main/java/org/jabref/logic/util/GitHubActionsEscape.java[15-25]
- Implement an AWK function equivalent to
GitHubActionsEscape.property(escape%,\r,\n, then additionally escape:and,). - Apply it to
filebefore printing. - (Optional) Also escape the message data portion if it ever becomes dynamic in the future.
[correctness] Wrong range for pure additions
Wrong range for pure additions
For pure-addition hunks (`oldCount==0`), the script still uses the *old* hunk range (`-oldStart,oldCount`) and can emit `line=0,endLine=0` (e.g., new files or insertions at the start), producing a meaningless “lines 0-0” message and an annotation that cannot reliably attach to source. For `oldCount==0`, the annotation should use the `+newStart,newCount` range (and clamp to 1-based lines).When a diff hunk has oldCount==0 (pure addition / new file), the script currently anchors the annotation using oldStart, which can be 0 and does not represent real lines in the committed file. This can yield line=0,endLine=0 and a confusing message.
The hunk header contains both old and new ranges: @@ -oldStart,oldCount +newStart,newCount @@. If oldCount==0, there are no old lines to annotate, so the correct anchor is the new range.
- .github/format-diff-annotations.sh[21-29]
- Parse
$3(+newStart,newCount) as well as$2. - If
oldCount > 0, keep current behavior (use old range). - Else (pure addition), set
start=newStartandend=newStart+newCount-1. - Ensure
start >= 1andend >= startbefore emitting the annotation.
[observability] Misleading OpenRewrite annotations
Misleading OpenRewrite annotations
The annotation title/text is hardcoded to “Formatting / Run the formatter”, but the workflow also uses this script to fail the OpenRewrite job when it leaves a dirty working tree. When OpenRewrite causes semantic changes or adds/removes files, the annotations will incorrectly tell contributors to “run the formatter” instead of indicating OpenRewrite output needs to be committed/fixed.The script always emits title=Formatting and a message telling users to run the formatter, but it is also invoked from the openrewrite job where the root cause may be OpenRewrite changes rather than formatting.
In .github/workflows/tests-code.yml, the OpenRewrite job runs :rewriteRun, then runs the formatter, then calls .github/format-diff-annotations.sh. Any remaining diff at that point is not necessarily a formatting issue.
- .github/workflows/tests-code.yml[108-129]
- .github/format-diff-annotations.sh[1-33]
- Add optional parameters or env vars to
.github/format-diff-annotations.sh(e.g.,ANNOTATION_TITLE/ANNOTATION_MESSAGE_PREFIX, or a first arg likeformat|openrewrite). - In the OpenRewrite job, set the title/message to something like
OpenRewrite/Uncommitted changes after OpenRewrite; run :rewriteRun and commit results. - Keep the current “Formatting” messaging for the format jobs.
| PR 15870 (2026-06-01) |
[correctness] Immutable exporters list
Immutable exporters list
ExportPreferences#getDefault passes List.of() into FXCollections.observableList, producing an ObservableList backed by an immutable list. Later calls to ExportPreferences#setCustomExporters/setAll will attempt to mutate that list (setAll), causing UnsupportedOperationException and breaking the Custom Exporters preferences flow (and other preference reset/import flows when no custom exporters are stored).ExportPreferences stores customExporters using FXCollections.observableList(customExporters). The default constructor passes List.of(), so the resulting ObservableList is backed by an immutable list. Any later mutation via setAll(...) / setCustomExporters(...) can throw UnsupportedOperationException.
This manifests in GUI when saving the “Custom Exporters” preferences tab because CustomExporterTabViewModel#storeSettings calls preferences.getExportPreferences().setCustomExporters(...), which calls customExporters.setAll(...).
It also affects normal startup/import/reset flows when no custom exporters are stored, because the backing-store loader returns defaults for an empty series.
Ensure customExporters is always backed by a mutable list, e.g.:
- In
ExportPreferencesconstructor, useFXCollections.observableArrayList(customExporters)orFXCollections.observableList(new ArrayList<>(customExporters)). - Keep
setCustomExporters/setAllas-is afterward.
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[33-50]
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[56-61]
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[99-105]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1682-1688]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1771-1779]
[maintainability] `get(0)` replaces `getFirst()`
`get(0)` replaces `getFirst()`
The updated export save-order persistence uses `List.get(0)` instead of the modern `SequencedCollection#getFirst()` idiom, regressing from a more modern Java style. This diverges from the project’s stated preference for modern Java 25+ APIs/idioms.Modern Java provides SequencedCollection#getFirst() for readability and consistency; the PR changed first-element access to get(0).
This code previously used getFirst() and the compliance checklist explicitly prefers modern Java 25+ idioms.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1694-1699]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1727-1731]
[maintainability] `keyWordDelimiterProperty` Javadoc wrong
`keyWordDelimiterProperty` Javadoc wrong
The updated parameter comment references `BibEntryProperties#keyWordDelimiterProperty`, which does not exist (and no longer matches the renamed `keywordSeparator` concept). This creates confusing and misleading documentation for maintainers.A Javadoc/parameter comment references a non-existent or outdated API name, making the documentation misleading.
The code now uses keywordSeparator, but the Javadoc still points to BibEntryProperties#keyWordDelimiterProperty.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyPatternPreferences.java[46-46]
| PR 15867 (2026-05-31) |
[reliability] Date parsing can crash
Date parsing can crash
ZoteroCitationMarkParser#setDate and helpers call List.getFirst() and Integer.parseInt() on unvalidated CSL "date-parts" contents, so empty inner arrays or non-numeric parts can throw NoSuchElementException/NumberFormatException. These exceptions are not caught by parse() (it only catches JsonParseException), so parsing can fail hard and bubble up to callers.ZoteroCitationMarkParser#setDate assumes issued.date-parts is well-formed and numeric. It uses getFirst() and Integer.parseInt() without validating inner list contents, which can throw runtime exceptions that are not caught by parse().
This parser is intended to consume Zotero-provided CSL JSON embedded in a reference mark name. Real-world data can be incomplete or partially missing (e.g., "date-parts":[[]]) or contain unexpected tokens; the parser should degrade gracefully (e.g., omit date fields) rather than throw.
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[39-50]
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[107-164]
- In
setDate(...): - Guard against
issuedData == null. - After
issuedData.datePartsnon-empty, also validate the first inner list is non-null and non-empty before callinggetFirst(). - Replace direct
Integer.parseInt(...)calls with safe parsing (e.g., try/catchNumberFormatExceptionand return/skip setting month/day), so malformed values don’t crash parsing. - Consider broadening the
parse(...)catch to includeRuntimeException(or at leastNoSuchElementException/NumberFormatException) and returnList.of()after logging at debug level. - Add tests for edge cases such as
"date-parts":[[]]and non-numeric date parts to prevent regressions.
| PR 15863 (2026-05-30) |
[correctness] `See what's new` typo
`See what's new` typo
The new Russian translation contains a spelling error (`Узнайть`) which will be shown to users in the UI. This violates the UI text style guideline to keep spelling consistent and professional.The Russian UI translation for See what's new contains a spelling typo (Узнайть).
This string is user-facing and should follow UI text style guidelines (correct spelling).
- jablib/src/main/resources/l10n/JabRef_ru.properties[1665-1665]
[correctness] `Move URL...` missing quote
`Move URL...` missing quote
The new Russian translation has unbalanced quotes around the field name `note` for the string `Move URL in 'note' field to 'URL' field`, which makes the Cleanup checkbox label render with malformed quoting and look unpolished to users. This violates the UI text style guideline (PR Compliance ID 20) requiring consistent, correct punctuation in user-facing UI text.The Russian translation for Move URL in 'note' field to 'URL' field is missing the closing ' after note, causing unbalanced quotes and a malformed label shown to users.
This is user-facing UI text and must have correct punctuation/quoting to comply with PR Compliance ID 20 (consistent, correct UI text). The key is used as a UI label in the Cleanup entries panel (FXML), and JabRef’s localization performs %0/%1 placeholder substitution but does not interpret quotes specially, so this is purely a visible string defect.
- jablib/src/main/resources/l10n/JabRef_ru.properties[1214-1214]
| PR 15862 (2026-05-30) |
[correctness] getDefaultPath override blocked
getDefaultPath override blocked
JabRefCliPreferences#getDefaultPath is now private, so JabRefGuiPreferences#getDefaultPath annotated with @Override no longer overrides and the project should not compile. Even if the annotation is removed, GUI-mode defaults (e.g., LAST_USED_DIRECTORY and FilePreferences defaults) will fall back to "/" instead of NativeDesktop’s default directory.JabRefCliPreferences#getDefaultPath() was changed from an overridable method to private, which prevents JabRefGuiPreferences from overriding it (and makes its existing @Override invalid). This breaks GUI-mode default directory behavior (and likely compilation).
JabRefGuiPreferences expects to override getDefaultPath() to use NativeDesktop.get().getDefaultFileChooserDirectory(), but JabRefCliPreferences now defines a private method returning /.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[952-954]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[1261-1265]
[reliability] Reset crashes clearing truststore
Reset crashes clearing truststore
`clear()` calls `clearTruststoreFromCustomCertificates()`, which constructs a `TrustStoreManager` using `defaults.get(SSL_TRUSTSTORE_PATH).toString()`. The PR removed initialization of `SSL_TRUSTSTORE_PATH` in the `defaults` map, so reset can throw a `NullPointerException` and abort.Preferences reset can crash with an NPE because clearTruststoreFromCustomCertificates() still reads the truststore path from defaults, but the PR removed the defaults.put(SSL_TRUSTSTORE_PATH, ...) initialization.
clear() always calls clearTruststoreFromCustomCertificates(). That method should not depend on a defaults entry that may not exist; it should use getSSLPreferences().getTruststorePath() (with a safe fallback) or SSLPreferences.getDefault().getTruststorePath().
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[749-752]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[488-596]
| PR 15838 (2026-05-27) |
[correctness] `csv.begin.layout` missing `Chapter` header
`csv.begin.layout` missing `Chapter` header
The new CSV header row omits standard BibTeX fields such as `chapter` and `annote`, so the exported CSV cannot include headers for all standard fields as required. This makes the export incomplete and inconsistent with the stated objective.The CSV header template does not include headers for all standard BibTeX fields (e.g., chapter, annote). This violates the requirement that csv.begin.layout emits a complete header row for all standard fields.
StandardField includes standard BibTeX fields like ANNOTE and CHAPTER, but the current csv.begin.layout header line does not include corresponding columns.
- jablib/src/main/resources/resource/layout/csv/csv.begin.layout[1-1]
- jablib/src/main/resources/resource/layout/csv/csv.layout[1-1]
[reliability] `GenericCsvExportFormatTest` lacks key test
`GenericCsvExportFormatTest` lacks key test
The added tests do not verify the full required header set and do not assert citation key export behavior, so they don’t validate the key requirements of the new CSV exporter. This reduces regression protection for the new export format.The new CSV exporter tests only check that the header contains a handful of substrings and never assert citation key output behavior. The compliance requirement expects automated tests to validate headers (for all standard fields) and citation key behavior.
performExportContainsAllStandardFieldHeaders currently asserts only a few header fragments, and the tests never set a citation key (e.g., via withCitationKey(...)) to validate how it is exported.
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[46-62]
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[93-111]
[correctness] Unescaped quotes in CSV
Unescaped quotes in CSV
The new `csv.layout` wraps most fields in double quotes but does not escape embedded `"` characters for those fields, producing malformed CSV when values contain quotes. This can break spreadsheet import and can also violate the template README’s “one row per entry” promise when fields contain line breaks.The new generic CSV template quotes values (e.g., "\author") but does not apply CSV escaping for embedded double quotes (and does not normalize newlines), so exported CSV can be malformed or span multiple lines per entry.
CSV requires embedded " inside quoted fields to be escaped as "". JabRef already has ReplaceWithEscapedDoubleQuotes for exactly this purpose, but the new template only applies it to \citationkey.
- jablib/src/main/resources/resource/layout/csv/csv.layout[1-1]
- jablib/src/main/resources/resource/layout/csv/README[1-4]
- jablib/src/main/java/org/jabref/logic/layout/format/ReplaceWithEscapedDoubleQuotes.java[5-18]
- Wrap each exported field in a formatter chain that at least applies
ReplaceWithEscapedDoubleQuotes(and ideally also replaces\nwith a space to keep one row per entry), e.g.: "\format[ReplaceWithEscapedDoubleQuotes]{\author}"- or
"\format[Replace(\n, ),ReplaceWithEscapedDoubleQuotes]{\abstract}" - Consider also quoting/escaping the citation key consistently (currently it is escaped but not quoted).
- Add/extend a unit test to include a field value containing a double quote (and optionally a newline) and assert the output is valid (e.g., contains doubled quotes).
[reliability] Missing exporter test lock
Missing exporter test lock
`GenericCsvExportFormatTest` lacks the `@Execution(SAME_THREAD)` / `@ResourceLock("exporter")` guards used by other exporter tests, so it may run concurrently and become flaky. This is especially risky because `TemplateExporter` mutates global `Number.serialExportNumber` during export.GenericCsvExportFormatTest can run in parallel with other exporter tests, but exporter code mutates global static state, which can lead to non-deterministic failures.
TemplateExporter sets and increments Number.serialExportNumber (a public static int) during export. Existing exporter tests (e.g., CsvExportFormatTest) protect against this by forcing same-thread execution and applying a shared ResourceLock("exporter").
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[18-33]
- jablib/src/main/java/org/jabref/logic/exporter/TemplateExporter.java[240-246]
- jablib/src/main/java/org/jabref/logic/layout/format/Number.java[7-19]
- jablib/src/test/java/org/jabref/logic/exporter/CsvExportFormatTest.java[31-33]
- Add the same annotations used by
CsvExportFormatTest: @Execution(ExecutionMode.SAME_THREAD)@ResourceLock("exporter")- and the required imports.
- (Optional) Add an
@AfterEachcleanup like the existing test class, for consistency.
| PR 15837 (2026-05-26) |
[correctness] Duplicate extension on invalid path
Duplicate extension on invalid path
When Path.of(fileName) throws InvalidPathException in getBaseName, getBaseName returns the full filename (including extension), but getValidFileName then appends the extracted extension again, producing results like "name.pdf.pdf".getValidFileName now always rebuilds the filename as cleanedBase + '.' + extension. If getBaseName hit InvalidPathException it returns the original fileName (which can still contain the extension), leading to duplicated extensions (<cleanedNameIncludingExt>.<ext>).
-
getFileExtensionwas updated to tolerateInvalidPathExceptionvia a fallback, so the invalid-path flow is explicitly supported. -
getBaseNamestill returns the rawfileNameonInvalidPathException, which is not a "base name". - The newly added test only asserts non-empty output for an invalid-path-like input, so it would not catch
.pdf.pdf.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[118-131]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[178-198]
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[409-413]
In getBaseName(String) catch block, compute a best-effort base name instead of returning the full input. For example:
- Use
FilenameUtils.getBaseName(FilenameUtils.getName(fileName.trim()))(matching the strategy used ingetFileExtension).
Update getValidFileNameDoesNotThrowForInvalidPathCharacters (or add a new test) to assert the output does not contain a duplicated extension (e.g., does not end with .pdf.pdf).
[reliability] Unhandled InvalidPathException
Unhandled InvalidPathException
FileUtil.getValidFileName now unconditionally calls getFileExtension(fileName), but getFileExtension uses Path.of(fileName.trim()) without handling InvalidPathException, so getValidFileName can now throw at runtime for inputs it is supposed to clean. This is a regression because getBaseName explicitly tolerates InvalidPathException and getValidFileName is used in file-export/rename flows where the input can originate from user/library data.FileUtil.getValidFileName(String) now always evaluates the extension via getFileExtension(fileName). getFileExtension(String) calls Path.of(fileName.trim()) without catching InvalidPathException, which means getValidFileName can throw for filenames containing characters invalid for Path parsing on the current OS (exactly the kind of inputs this method exists to sanitize).
-
getBaseName(String)already catchesInvalidPathExceptionand falls back to the raw string, implying invalid path strings are expected/possible. -
getValidFileName(String)is used in export and linked-file flows, so an exception here can break rename/export operations.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[76-97]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[112-125]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[172-188]
- Make
getFileExtension(String)resilient by catchingInvalidPathExceptionand extracting the “name” portion withoutPath.of(e.g.,FilenameUtils.getName(trimmed)), then proceed withFilenameUtils.getExtension(...). - This keeps extension extraction consistent for all callers.
- Alternatively (less ideal), wrap the
getFileExtension(fileName)call insidegetValidFileNamewith try/catch and fall back to a string-based extension parse on failure. - Add a unit test that passes a name containing characters that would be invalid for
Path.ofon Windows (e.g.,"a:b.pdf") and assertgetValidFileNamereturns a sanitized value without throwing.
[maintainability] `Stream.generate` for repeat string
`Stream.generate` for repeat string
The new test builds long strings using `Stream.generate(...).limit(...).collect(Collectors.joining())` where the modern and clearer Java API `"1".repeat(n)` would be appropriate. This introduces an outdated pattern in newly added code and reduces readability.The new test constructs repeated-character strings using Stream.generate(...).collect(Collectors.joining()), which is less idiomatic than String#repeat(int) on modern Java.
The project encourages using modern Java (25+) idioms where applicable to improve readability/maintainability.
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[401-408]
[maintainability] Misaligned chain indentation
Misaligned chain indentation
The newly added test uses deeply indented chained-call formatting that deviates from the surrounding method-chaining style in the same test class. This introduces unnecessary formatting inconsistency in modified areas.The new test’s method-chain indentation is inconsistent with the existing formatting in FileUtilTest, creating a noisy/less consistent style.
This PR should preserve established formatting conventions and avoid unnecessary reformatting/stylistic divergence.
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[401-408]
[correctness] Negative truncation length
Negative truncation length
When truncating, the computed substring length can become negative if the extension length is very large, causing StringIndexOutOfBoundsException in getValidFileName. The new full-filename length check makes this branch reachable even when the base name is short but the extension is extremely long.getValidFileName computes MAXIMUM_FILE_NAME_LENGTH - (extensionLength + 1) and uses that directly as the substring end index. If the extension is extremely long, this value can be negative and substring(0, negative) throws.
This is an edge case (very long extensions are unusual), but the failure mode is a hard exception in a core utility method.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[172-188]
- Guard the computed max base length:
int maxBaseLen = MAXIMUM_FILE_NAME_LENGTH - extension.map(s -> s.length() + 1).orElse(0);- If
maxBaseLen <= 0, avoid callingsubstringwith a negative index. - Simple safe fallback: return
fullCleanedName.substring(0, MAXIMUM_FILE_NAME_LENGTH)whenmaxBaseLen <= 0. - (Alternative: truncate the extension to fit, but any safe non-throwing behavior is better than an exception.)
- Add a unit test with an extension length > 254 to assert no exception and the result length is <= 255.
| PR 15835 (2026-05-26) |
[maintainability] Duplicated `mutationScheduler` change code
Duplicated `mutationScheduler` change code
Two cleanup jobs duplicate the same `ArrayList` + `mutationScheduler.accept(() -> entry.setFiles(...))` pattern, increasing maintenance cost and the risk of inconsistent future fixes. This violates the no-duplication requirement.MoveFilesCleanup and RenamePdfCleanup both contain a duplicated snippet that (1) creates a mutable list, (2) schedules an entry mutation via mutationScheduler, and (3) returns the list. This should be centralized to avoid duplication and keep behavior consistent.
Both classes are CleanupJob implementations that need to schedule BibEntry mutations through mutationScheduler while returning List<FieldChange>. The duplicated pattern can be replaced by a shared helper (e.g., a reusable utility method or a generic helper in CleanupJob) that runs a mutation via the scheduler and returns the produced List<FieldChange>.
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[53-56]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[59-63]
[performance] FX-thread file serialization
FX-thread file serialization
MoveFilesCleanup and RenamePdfCleanup call BibEntry#setFiles(files) inside mutationScheduler, which in the GUI is UiTaskExecutor::runAndWaitInJavaFXThread, so FILE-field serialization runs on the JavaFX thread. This can cause noticeable FX-thread stalls for entries with many linked files or long paths, even though only the actual field mutation needs to be scheduled.MoveFilesCleanup and RenamePdfCleanup schedule entry.setFiles(files) on mutationScheduler. In the GUI cleanup flow, this scheduler is UiTaskExecutor::runAndWaitInJavaFXThread, which means BibEntry#setFiles (and its FileFieldWriter.getStringRepresentation(files) serialization work) runs on the JavaFX thread.
This PR correctly moved the actual BibEntry mutation onto the scheduler thread to avoid “not on FX thread” exceptions. However, setFiles(...) does both (1) computation/serialization and (2) the mutation, so scheduling it also schedules the computation.
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[39-66]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[36-72]
- Keep file I/O and any string construction on the calling (background) thread.
- Precompute the new FILE field string (e.g., via
FileFieldWriter.getStringRepresentation(files)) and compare to the current value off-FX. - Only schedule the minimal mutation on the FX thread, e.g.
entry.setField(StandardField.FILE, newValue)(or equivalent), and capture the returnedFieldChangevia anAtomicReferenceif you need to return it. - Apply the same pattern in both
MoveFilesCleanupandRenamePdfCleanup.
| PR 15832 (2026-05-26) |
[reliability] Heylogs failure masked
Heylogs failure masked
The CHANGELOG lint step forces success with `|| true` and only fails based on counting `::error` lines in `heylogs.txt`, so if jbang/heylogs fails without emitting GitHub Actions `::error` annotations to stdout (e.g., stderr-only failure), CI will pass without validating CHANGELOG.md.The workflow masks the exit code of the heylogs invocation (|| true) and decides pass/fail only by grepping ::error lines in heylogs.txt. If heylogs/jbang fails without producing those annotations on stdout (common for runtime failures which go to stderr), error_count becomes 0 and the step passes, resulting in false-green CI.
You still want to ignore the specific heylogs rule all-h2-contain-a-version because of the Unreleased section, but you should not ignore unexpected command failures.
- .github/workflows/tests-code.yml[188-200]
- Capture the pipeline exit code (via
PIPESTATUS) and fail on unexpected non-zero exit codes, while still allowing the lint-error path to be handled by parsing::errorlines. - Also consider capturing stderr into
heylogs.txt(2>&1) so runtime failures are visible and can be surfaced. Example pattern:
| PR 15830 (2026-05-26) |
[correctness] Git options bypass guard
Git options bypass guard
`find_violation()` assumes the git subcommand is the token immediately after `git`, so any global git option placed there (e.g., `git -C … rebase`) will bypass the rebase/force-push blocking entirely. This undermines the PR’s core goal of reliably forbidding rebase/force-push for AI agents.The hook assumes tokens[i+1] is the git subcommand, which fails when git global options appear between git and the subcommand.
Git invocations can legally include global options before the subcommand; the hook should identify the actual subcommand before deciding whether to deny.
- .claude/hooks/git-rebase-guard.py[18-53]
- When you encounter a
gittoken, scan forward past recognized global options (and their arguments where applicable, e.g.-C <dir>,-c <name>=<value>,--git-dir=<path>,--work-tree=<path>) until the first non-option token, and treat that as the subcommand. - Then apply the existing
rebase/pull/pushchecks against that subcommand and the remaining args.
[correctness] Pull -r bypass allowed
Pull -r bypass allowed
The hook only blocks `git pull` when it sees `--rebase`/`--rebase=…`, so other rebase-enabling pull variants (e.g., `-r`) will not be denied. This allows rebase-based pulls to proceed despite the stated policy intent.The git pull check only detects --rebase and --rebase=... tokens, missing other rebase-enabling variants.
To reliably forbid rebase-based pulls, the hook should treat common short/variant flags as equivalent to --rebase.
- .claude/hooks/git-rebase-guard.py[35-41]
- Extend the predicate to also match
-r. - Consider matching any token that starts with
--rebase(e.g.,--rebase-merges) if the policy is “no rebase at all.”
[security] Force push refspec bypass
Force push refspec bypass
The hook blocks force-push only when `--force`/`--force-with-lease`/`-f` is present, but force-updates can also be expressed via `+`-prefixed refspecs that won’t be detected. This leaves a direct path to perform a forbidden force push without triggering the guard.The push guard only checks for force flags and ignores refspec arguments, so +-prefixed refspec force-updates are not blocked.
To enforce “no force push,” the hook must also inspect non-flag arguments to git push.
- .claude/hooks/git-rebase-guard.py[43-52]
- After identifying
git push, scan remaining args for refspec tokens beginning with+(e.g.,+HEAD:branch,+refs/heads/x:refs/heads/y) and deny if any are found. - Optionally also include other force-like flags (if relevant to your policy) alongside the existing set.
[maintainability] Docs/hook pull mismatch
Docs/hook pull mismatch
AGENTS.md says “Do not use bare `git pull`… Use the explicit `fetch` + `merge` pair,” but the hook does not deny plain `git pull`. This creates inconsistent guidance vs. enforcement for AI agents.Documentation forbids bare git pull, but the enforcement hook only blocks git pull when rebase is explicitly requested.
This mismatch can confuse agents (and humans) about what will be blocked vs. allowed.
- AGENTS.md[440-452]
- .claude/hooks/git-rebase-guard.py[35-41]
Choose one:
- Enforce the doc: deny any
git pullinvocation (regardless of flags) and instruct to usefetch+merge. - Or adjust AGENTS.md wording to clarify the hook only blocks explicit rebase pulls, if that’s the intended enforcement scope.
| PR 15827 (2026-05-25) |
[correctness] `withMainFileDirectory` sets wrong property
`withMainFileDirectory` sets wrong property
`FilePreferences.withMainFileDirectory` incorrectly assigns the provided path to `workingDirectory` (and even uses a misleading parameter name), so callers attempting to reset/import the main file directory do not actually update it and may instead overwrite the working directory setting. This breaks expected preference reset behavior (including the GUI reset path) and makes the API naming inconsistent and misleading.FilePreferences.withMainFileDirectory(...) currently updates workingDirectory instead of mainFileDirectory, and its parameter name (lastUsedDirectory) is misleading. As a result, callers (notably preference reset via JabRefCliPreferences.clear() used by the GUI reset path) silently fail to reset the main file directory and may unintentionally overwrite the working directory.
- The method is intended (by name and usage) to set the main file directory, but it assigns the provided
Pathto the wrong field. -
JabRefCliPreferences.clear()usesFilePreferences.getDefault().withMainFileDirectory(getDefaultPath())to reset the main file directory. - In GUI mode,
getDefaultPath()is overridden to the default file chooser directory, so this bug directly breaks the expected GUI preferences reset behavior. - The compliance checklist expectation of small, focused, naming-consistent code is violated because the method name/purpose and implementation disagree.
- jablib/src/main/java/org/jabref/logic/FilePreferences.java[384-387]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[991-996]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[1224-1228]
[reliability] Tests mock old return
Tests mock old return
FilePreferences.getMainFileDirectory() now returns Path, but some tests still mock it as Optional, which will fail compilation and block the build.The API change from Optional<Path> getMainFileDirectory() to Path getMainFileDirectory() was not propagated to all tests; remaining mocks returning Optional.of(...) no longer match the method signature.
Two test files still return Optional<Path> from getMainFileDirectory(), which should now return Path.
- jablib/src/main/java/org/jabref/logic/FilePreferences.java[142-152]
- jabgui/src/test/java/org/jabref/gui/desktop/os/BibDatbaseContextTest.java[27-36]
- jabgui/src/test/java/org/jabref/gui/fieldeditors/contextmenu/ContextMenuFactoryTest.java[58-65]
- Replace
thenReturn(Optional.of(path))withthenReturn(path). - Remove now-unused
Optionalimports/usage in those tests.
[correctness] Blank directory becomes CWD
Blank directory becomes CWD
LinkedFilesTabViewModel stores the main file directory via Path.of(text) and the validator does not reject blank input, so clearing the field can produce an empty Path that resolves relative to the current working directory and is treated as valid.The linked-files preferences UI converts the text field to a Path using Path.of(...) without handling blank input. An empty string yields an "empty" path; the validator only checks Files.exists/isDirectory and therefore can accept the current working directory when the field is cleared.
This behavior is new/impactful because storeSettings() now parses the path immediately (instead of storing a raw string), and the validator currently has no explicit blank-string guard.
- jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java[64-80]
- jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java[116-120]
- In the validator: if the value is
null/blank, return a validation error (or normalize to the desired default). - In
storeSettings(): guard against blank input; either - normalize blank to the default main directory (e.g., preferences default), or
- prevent saving and rely on validation to block closing the dialog.
- Add a regression test covering blank input behavior.
| PR 15823 (2026-05-24) |
[maintainability] `AuthorListParser` formatting-only changes
`AuthorListParser` formatting-only changes
Several changes in `AuthorListParser` appear to be formatting-only (notably unusually wide indentation in `Set.of(...)` declarations and comment/Javadoc reflow) and unrelated to the functional fix, creating noisy diffs and reducing readability. This violates the requirement to avoid reformatting and keep changes minimal and consistent with the existing style.AuthorListParser.java includes formatting-only changes (especially unusually wide indentation in Set.of(...) declarations and comment/Javadoc reflow) that are not required for the behavioral fix, creating unnecessary diff noise and deviating from the existing formatting style.
The PR’s functional change is in token case detection, but multiple unrelated formatting edits were introduced, including re-indentation of the AVOID_TERMS_IN_LOWER_CASE and TEX_NAMES Set.of(...) constants and reflow of comments/Javadoc into long single lines. These should be reverted or adjusted back to the standard continuation indentation used across the codebase to reduce churn and improve readability.
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[23-26]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[36-39]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[111-116]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[200-201]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[420-426]
[maintainability] `CHANGELOG` mentions `namePrefix`
`CHANGELOG` mentions `namePrefix`
The new changelog bullet is written with internal implementation terms (e.g., `namePrefix`, `familyName`) and starts with lowercase “we,” which makes it less end-user focused and inconsistent with the existing `- We ...` capitalization/style used in surrounding entries. This violates the changelog requirement for user-oriented language and consistent formatting in release notes.The added CHANGELOG.md entry is not end-user oriented (it uses internal implementation terminology like namePrefix/familyName) and it breaks the existing bullet style by starting with lowercase “we” instead of matching the surrounding - We ... capitalization.
This is user-facing release-note text in the ### Fixed section; surrounding bullets consistently start with - We fixed ... and avoid internal code-level field names. The current entry uses lowercase we and references internal terms (namePrefix, familyName), making it inconsistent with the established changelog style and less readable for end users.
- CHANGELOG.md[31-38]
- CHANGELOG.md[37-37]
[maintainability] Misleading tokenCase logic
Misleading tokenCase logic
`getToken()` now sets `tokenCase` true for any letter that is not in Unicode category `LOWERCASE_LETTER`, which no longer matches the in-file documentation (“true if upper-case token”) and makes the condition redundant/unclear. This increases the risk of future incorrect changes because `tokenCase` drives von/last-name splitting decisions.tokenCase is documented as “true if upper-case token, false if lower-case”, but the new condition effectively treats any non-lowercase letter (including caseless scripts) as true. The current expression is also redundant: under the existing Character.isLetter(c) guard, Character.getType(c) != Character.LOWERCASE_LETTER already covers upper-case and Han, making isUpperCase/HAN checks unnecessary.
tokenCase is later used to decide whether a token starts the “von part” (!tokenCase) and where the last name begins (tokenCase), so readability and correct semantics matter.
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[54-57]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[467-475]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[230-276]
- Replace the assignment with something explicit and self-documenting, e.g.:
tokenCase = !Character.isLowerCase(c);- or keep
getTypebut simplify to:tokenCase = Character.getType(c) != Character.LOWERCASE_LETTER; - Update the field comment to match the new semantics, e.g. “true if token does not start with a lowercase letter (uppercase/caseless)”.
- (Optional) add a small comment explaining the caseless-script rationale so the intent is preserved.
| PR 15810 (2026-05-22) |
[maintainability] Blank line before `req~`
Blank line before `req~`
In `docs/requirements/slr.md`, each requirement heading is separated from its OpenFastTrace `req~...` identifier line by a blank line, violating the repository’s required requirements authoring format. This can impair automated requirement extraction/tracing and reduce traceability consistency.The OpenFastTrace/requirements authoring convention requires the requirement identifier line (e.g., req~...~1) to appear immediately under the Markdown heading with no blank line in between. In docs/requirements/slr.md, an empty line is placed between each ## ... heading and its req~... identifier, which can break or confuse automated requirement extraction and tracing.
PR Compliance ID 33 and the repo’s requirements guide explicitly state that IDs must be directly below headings with no empty line. Other requirement documents under docs/requirements/ follow this pattern, and CI runs gradle traceRequirements, so consistent formatting is required for tooling to parse requirements reliably.
- docs/requirements/slr.md[19-41]
[reliability] Markdown trailing whitespace
Markdown trailing whitespace
`docs/requirements/slr.md` contains a trailing space at the end of a list item, which violates markdownlint MD009 and can fail the CI Markdown job. This will block merging even though the change is only documentation.A trailing space at end-of-line violates markdownlint (MD009).
The CI workflow runs markdownlint over docs/**/*.md, and .markdownlint.yml keeps default rules enabled.
- docs/requirements/slr.md[13-13]
| PR 15789 (2026-05-20) |
[maintainability] Changelog entry lacks PR link
Changelog entry lacks PR link
The new CHANGELOG bullet for the `github-actions` output format does not include an issue/PR reference link, unlike surrounding entries. This reduces traceability and consistency in user-facing documentation.The newly added changelog entry is missing the required issue/PR reference, reducing traceability and making the changelog inconsistent.
In CHANGELOG.md, entries should reference an issue (#NUM) or link the PR implementing the change.
- CHANGELOG.md[20-20]
| PR 15780 (2026-05-19) |
[reliability] Skip import on adjust error
Skip import on adjust error
If `adjustLinkedFilesForTargetIfRequired` fails, the `onFailure` path marks the entry as skipped and never calls `importCleanedEntries`, so the entry is not imported at all even though only linked-file adjustment failed. This can drop entries on common filesystem/path issues during transfer (e.g., invalid paths or unexpected runtime exceptions in file operations).ImportHandler#importEntryWithDuplicateCheck runs linked-file adjustment in a background task. If that task fails, it marks the entry as skipped and does not import it, causing silent entry loss when adjustment fails.
adjustLinkedFilesForTargetIfRequired calls LinkedFileTransferHelper.adjustLinkedFilesForTarget, which performs path and filesystem operations. While many IO failures are handled internally, uncaught runtime exceptions (e.g., invalid path strings) can still happen and currently abort the entry import.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[286-296]
In the .onFailure(...) of the linked-file adjustment task:
- still proceed with
importCleanedEntries(transferInformation, List.of(entryForImport))(or the best available unadjusted entry) - call
tracker.markImported(...)(or mark as imported-with-warning) - optionally show a user-visible warning/notification instead of only logging
[reliability] Dialog lock likely ineffective
Dialog lock likely ineffective
`getDuplicateDecision` uses `synchronized` to prevent multiple duplicate resolver dialogs, but all `BackgroundTask` success callbacks are invoked on the JavaFX thread, so the lock does not serialize same-thread dialog requests. Because the dialog is shown via `Dialog.showAndWait()`, which runs a nested event loop, other queued FX callbacks can still reach `getDuplicateDecision` while a dialog is open, allowing additional dialogs to open before the first decision is made.The PR adds synchronized (duplicateDecisionLock) around dialogService.showCustomDialogAndWait(dialog) to prevent multiple dialogs. However, duplicate handling runs on the JavaFX thread, and showCustomDialogAndWait calls Dialog.showAndWait() which runs a nested event loop; additional FX callbacks can still fire and re-enter getDuplicateDecision.
Multiple entries are imported concurrently (importEntriesWithDuplicateCheck starts tasks in a loop). Several entries can discover duplicates and attempt to resolve them while the first dialog is still open.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[339-362]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[502-520]
Pick one approach:
- Batch-serialize duplicate resolution: process entries sequentially for the duplicate-resolution phase (chain background tasks) so only one dialog can be requested at a time.
- Queue dialog requests: maintain a single in-flight decision request and queue subsequent duplicate checks until the dialog completes, then resume with the chosen decision.
- If keeping a lock, replace it with an explicit non-reentrant “dialog in progress” gate + queue;
synchronizedalone does not prevent re-entry on the FX thread.
| PR 15779 (2026-05-19) |
[correctness] Crossref KEY inheritance broken
Crossref KEY inheritance broken
BibEntry#getSourceField now treats StandardField.KEY as a forbidden field, so BibEntry#getResolvedFieldOrAlias(StandardField.KEY, db) will no longer inherit "key" from the crossref parent. This contradicts the existing CrossrefTest expectations that StandardField.KEY is inherited via same-name inheritance, likely breaking crossref resolution behavior and tests.BibEntry#getSourceField was changed to return Optional.empty() for StandardField.KEY. This disables crossref inheritance for the BibTeX key field.
The repo’s crossref tests indicate StandardField.KEY is expected to be inherited (same-name inheritance). With the new forbidden check, resolving KEY via getResolvedFieldOrAlias(..., db) will return empty, causing behavior change and likely test failures.
-
InternalField.KEY_FIELDis the citation key;StandardField.KEYis a normal BibTeX field (used for sorting in some styles). Even if it’s “rare”, the codebase currently expects it to participate in crossref inheritance.
- jablib/src/main/java/org/jabref/model/entry/BibEntry.java[188-198]
Remove StandardField.KEY from the forbidden-fields list in getSourceField (or, if the intended behavior is to stop inheriting it, update the crossref behavior/tests accordingly and document the breaking change).
| PR 15775 (2026-05-19) |
[maintainability] PR template instructs emoji praise
PR template instructs emoji praise
The PR template now instructs adding a praise sentence ending with a `⭐` emoji, which is unprofessional and inconsistent for contributor-facing documentation. This can reduce the seriousness and clarity of PR descriptions.The PR template includes an instruction to add a sentence praising JabRef and end it with a ⭐ emoji. This is not professional guidance for contributor documentation and may encourage low-signal PR descriptions.
This change is in .github/PULL_REQUEST_TEMPLATE.md under the ### PR Description guidance.
- .github/PULL_REQUEST_TEMPLATE.md[32-33]
| PR 15773 (2026-05-19) |
[correctness] Wrong `JabRef_en.properties` path
Wrong `JabRef_en.properties` path
The new “Removing an obsolete key” section in `docs/code-howtos/localization.md` tells contributors to edit `src/main/resources/l10n/JabRef_en.properties`, but the repository’s actual English localization bundle is `jablib/src/main/resources/l10n/JabRef_en.properties`. Following the documentation as written can lead to editing/creating the wrong file, conflicting with the localization workflow and potentially leaving `LocalizationConsistencyTest` failing.The “Removing an obsolete key” instructions in docs/code-howtos/localization.md currently direct contributors to edit src/main/resources/l10n/JabRef_en.properties, but the correct English localization file in this repository is jablib/src/main/resources/l10n/JabRef_en.properties. Update the documentation so contributors modify the correct file path and don’t accidentally edit/create a non-existent file or leave localization tests failing.
- The localization workflow (PR Compliance ID 19) requires key additions/removals to be applied to
jablib/src/main/resources/l10n/JabRef_en.properties. - The document already references the correct
jablib/.../JabRef_en.propertiespath earlier, but the newly added “Removing an obsolete key” step usessrc/main/resources/..., creating an internal inconsistency.
- docs/code-howtos/localization.md[57-69]
[maintainability] Conflicting test class name
Conflicting test class name
`localization.md` now tells readers to run `org.jabref.logic.l10n.LocalizationConsistencyTest` in the new removal flow, but the existing add-key flow still references `org.jabref.logic.LocalizationConsistencyTest` (wrong package). This inconsistency can send contributors to a non-existent test class and block the workflow the doc is trying to describe.docs/code-howtos/localization.md contains conflicting instructions for which test to run:
- “Adding a new key” references
org.jabref.logic.LocalizationConsistencyTest(incorrect). - The new “Removing an obsolete key” section references
org.jabref.logic.l10n.LocalizationConsistencyTest(correct). This inconsistency is confusing and can cause contributors to try to run a test class that doesn’t exist.
The actual test class is in package org.jabref.logic.l10n.
- docs/code-howtos/localization.md[55-67]
- jablib/src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java[1-5]
| PR 15762 (2026-05-18) |
[reliability] Null selected tab deref
Null selected tab deref
LibraryTab#setDatabaseContext calls selectedItemProperty().get().equals(this), which can throw NullPointerException when the TabPane has no selected item. This can abort library loading and leave the StateManager in an inconsistent state.LibraryTab#setDatabaseContext uses tabPane.getSelectionModel().selectedItemProperty().get().equals(this) which will throw if the selected item is null.
Other code already accounts for TabPane having no selected tab (getSelectedItem() == null), so null is a real state and should be handled here as well.
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[372-382]
Replace the dereference with a null-safe comparison, e.g.:
| PR 15759 (2026-05-18) |
[correctness] Integrity exit code wrong
Integrity exit code wrong
CheckIntegrity.execute() always returns 0 even when integrity findings exist, so `jabkit check integrity` and `jabkit check FILE` cannot signal findings via exit status. This contradicts the parent `check` command’s documented exit-code semantics and can let CI pass with integrity problems.CheckIntegrity.execute(...) collects integrity messages, writes them, and then unconditionally returns 0. This prevents callers (including jabkit check FILE) from using exit codes to detect integrity findings.
The parent check command explicitly documents 1 = findings and combines exit codes using Math.max(...), but CheckIntegrity.execute(...) never returns 1.
- jabkit/src/main/java/org/jabref/toolkit/commands/CheckIntegrity.java[83-112]
- jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[50-54]
- After writing findings, return
messages.isEmpty() ? 0 : 1. - Update the
executeJavadoc to include1 = findingsfor consistency withCheckandCheckConsistency.
[correctness] Key findings misformatted
Key findings misformatted
IntegrityCheckResultErrorFormatWriter appends a field segment for citation-key findings emitted with `StandardField.KEY`, but requirements state citation-key (entry-level) findings must carry only the citation key. This breaks the documented `citationKey[:field]` shape and can confuse parsers consuming the errorformat stream.IntegrityCheckResultErrorFormatWriter only suppresses the field name when field == InternalField.KEY_FIELD. Some citation-key findings use StandardField.KEY, so the writer appends :key even though the requirement says entry-level findings on the citation key must not include a field segment.
CitationKeyDuplicationChecker emits IntegrityMessage(..., StandardField.KEY) for duplicate citation keys.
- jablib/src/main/java/org/jabref/logic/integrity/IntegrityCheckResultErrorFormatWriter.java[23-41]
- jablib/src/main/java/org/jabref/logic/integrity/CitationKeyDuplicationChecker.java[21-32]
- docs/requirements/cli.md[28-36]
- Treat
StandardField.KEYthe same asInternalField.KEY_FIELDfor formatting (do not append:field). - Optionally, for key-level findings use
parserResult.getCompleteEntryIndicator(entry)for the range, to avoid depending on field-alias resolution forStandardField.KEY.
[maintainability] Unlocalized `Check` console message
Unlocalized `Check` console message
`Check.call()` prints a hardcoded user-facing message instead of using the project localization mechanism. This prevents translation and violates the localization standard for user-visible messages.A user-facing CLI message is printed as a hardcoded English string, bypassing JabRef localization.
Other jabkit commands print user-visible output via Localization.lang(...). The new check command should follow the same convention.
- jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[44-46]
[reliability] JBang main job missing setup
JBang main job missing setup
The `jbang-main` workflow job invokes a local composite action without checking out the repository or installing JDK/JBang, so it will fail on `main`. The same workflow’s `jbang-pr` job still performs checkout and setup, making this a main-branch-only regression.The jbang-main job calls ./.github/actions/jbang-check but does not run actions/checkout and does not install JDK/JBang (previously done via ./.github/actions/setup-gradle). Local composite actions require the repo to be present in the workspace, and the jbang-check action assumes jbang is available.
jbang-pr still performs checkout and uses setup-gradle (which installs JDK and JBang), but jbang-main no longer does.
- .github/workflows/tests-code.yml[381-388]
- .github/actions/jbang-check/action.yml[1-25]
- .github/actions/setup-gradle/action.yml[14-44]
[observability] PdfUpdate errors on stdout
PdfUpdate errors on stdout
PdfUpdate prints fatal import/parse errors to stdout while returning non-zero exit codes, mixing errors into the normal output stream. Other commands in this PR route equivalent failures to stderr, so PdfUpdate remains inconsistent for scripting and tooling.PdfUpdate.call() prints file-open / invalid-input failures using System.out.println(...) even though these are fatal errors (exit code 2). This makes it harder to separate normal output from errors in pipelines.
Several other commands updated in this PR already use System.err.println(...) for the same error paths.
- jabkit/src/main/java/org/jabref/toolkit/commands/PdfUpdate.java[69-83]
- jabkit/src/main/java/org/jabref/toolkit/commands/Convert.java[59-68]
- Replace the two
System.out.println(...)calls in the error paths withSystem.err.println(...). - (Optional) Consider reusing the shared
importBibtexLibrary(...)helper to standardize message + exit-code handling.
[maintainability] `CheckConsistency` uses Optional.get()
`CheckConsistency` uses Optional.get()
The modified consistency- and integrity-check flows use `Optional.isEmpty()` followed by `Optional.get()`, which is discouraged by the Optional-control-flow compliance rule. This pattern is unnecessarily verbose and increases the risk of unsafe `get()` usage during future edits or refactors.New/modified code uses Optional.isEmpty() and then Optional.get(), violating the Optional-control-flow compliance rule.
PR Compliance ID 29 requires idiomatic Optional control flow (e.g., map, ifPresentOrElse, orElseThrow) instead of manual presence checks followed by get(), because the latter is verbose and can be accidentally made unsafe during later changes.
- jabkit/src/main/java/org/jabref/toolkit/commands/CheckConsistency.java[53-83]
- jabkit/src/main/java/org/jabref/toolkit/commands/CheckIntegrity.java[64-105]
| PR 15755 (2026-05-18) |
[correctness] Wrong CLI option description
Wrong CLI option description
`--importToOpen` is described as “Same as --importBibtex”, but `importToOpen` actually appends a file/URL to the current library while `--importBibtex` appends a literal BibTeX string. This mismatch will lead callers to pass the wrong argument type and trigger import failures (or confusion), especially for the native host script that uses `--importToOpen` with a temp file path.The help/description string for the picocli option --importToOpen claims it is the same as --importBibtex, but the code paths take different input types:
-
--importToOpenexpects a file path or URL to import. -
--importBibtexexpects a BibTeX string. This makes the CLI usage message incorrect and can cause wrong usage.
ArgumentProcessor routes these two options to different UiCommands, and the Windows native host script demonstrates that --importToOpen is invoked with a tempfile path.
- jabgui/src/main/java/org/jabref/cli/GuiCommandLine.java[19-28]
| PR 15750 (2026-05-17) |
[correctness] ISSN test missing assertion
ISSN test missing assertion
`exportsIssnNestedUnderSerialNumber` builds an `expected` YAML output but never asserts it against the exported file contents, so ISSN serialization is not actually covered and regressions could slip through. This violates the requirement to have unit tests validating ISSN is nested under `serial-number.issn` (and not emitted as a top-level field).The test exportsIssnNestedUnderSerialNumber currently constructs an expected YAML output but never compares it to the actual exporter output, meaning it cannot fail and does not validate that ISSN is nested under serial-number rather than emitted as a top-level YAML field.
PR Compliance requires unit tests for DOI/ISBN/ISSN export verifying the identifiers are nested under serial-number and not emitted as top-level YAML fields. In the same test class, the DOI and ISBN tests use assertEquals(expected, Files.readAllLines(file));, but the ISSN test ends right after creating expected without asserting the file contents.
- jablib/src/test/java/org/jabref/logic/exporter/HayagrivaYamlExporterTest.java[309-333]
| PR 15747 (2026-05-17) |
[maintainability] Changelog entry wording unpolished
Changelog entry wording unpolished
The new CHANGELOG entry has grammatical issues (`newly added entry cannot be searched`) and reads unpolished for end users. This reduces professionalism and clarity in user-facing release notes.The newly added CHANGELOG bullet is grammatically incorrect/unpolished and should be rewritten in professional, end-user-friendly language.
Compliance requires professional and consistent user-facing documentation.
- CHANGELOG.md[20-24]
[maintainability] `hashcode` misspelled in comment
`hashcode` misspelled in comment
The newly added comment uses `hashcode` instead of `hashCode`, which violates spelling/style conventions and reduces readability. Comments should follow standard Java terminology.A newly added comment misspells Java terminology as hashcode.
JabRef code style requires correct spelling/terminology in modified code and comments.
- jablib/src/main/java/org/jabref/logic/search/IndexManager.java[184-186]
| PR 15744 (2026-05-15) |
[maintainability] `AGENTS.md` promotes `Optional.get()`
`AGENTS.md` promotes `Optional.get()`
The updated guidance tells contributors to use `Optional.get()` when the value is present, which encourages unsafe and non-idiomatic Optional handling. This conflicts with the project’s requirement to use idiomatic Optional control flow (e.g., `ifPresent`, `ifPresentOrElse`, `map`, `orElseThrow`) rather than presence checks and `get()`.AGENTS.md currently advises using Optional.get() “if really present”, which is discouraged by the compliance rules favoring idiomatic Optional control flow.
This guidance is used to steer contributors/agents; recommending get() can lead to fragile code patterns and conflicts with the project’s Optional conventions.
- AGENTS.md[164-165]
[maintainability] `CHECLIST.md` broken reference
`CHECLIST.md` broken reference
AGENTS.md instructs readers to follow `CHECLIST.md`, but the PR adds `CHECKLIST.md`, making the reference incorrect and pointing to a non-existent document. This creates a broken documentation reference that can mislead contributors/agents and undermine checklist enforcement.AGENTS.md references CHECLIST.md (typo), but the checklist file added/present in the repository is CHECKLIST.md. This creates a broken/incorrect documentation reference that prevents readers/agents from finding and following the checklist.
The new guidance in AGENTS.md is intended to be followed after code changes, and this PR adds a new checklist file at the repository root; a wrong filename makes the instruction fail and undermines checklist enforcement.
- AGENTS.md[19-19]
[reliability] Broken docker mount variable
Broken docker mount variable
The documented formatter command uses `$pwd` for the Docker volume mount, but the surrounding examples use Unix-style commands; in typical bash/zsh terminals `$pwd` is not defined, so the container won’t mount the repo directory correctly and formatting won’t run on the intended files.The Docker formatting command uses $pwd which is PowerShell-specific. In bash/zsh (consistent with the nearby ./gradlew examples), this variable is usually unset, causing an incorrect -v mount.
The same command is duplicated in CHECKLIST.md, so it should be updated there too.
- AGENTS.md[341-342]
- CHECKLIST.md[6-8]
[maintainability] Misleading JSpecify checklist
Misleading JSpecify checklist
CHECKLIST.md says to “Use JSpecify instead of `== null`”, but the project’s nullness ADR describes JSpecify as progressive API annotation and the codebase still uses explicit null checks; this checklist item is phrased as a hard replacement rule that contradicts established practice.The checklist item implies JSpecify replaces runtime null checks, which is inaccurate/misleading. JSpecify annotations complement runtime checks by documenting/validating nullness contracts; explicit null checks remain common and necessary in implementations.
The project ADR explicitly frames JSpecify usage as progressive annotation of APIs for compile-time checking.
- CHECKLIST.md[6-6]
| PR 15743 (2026-05-15) |
[correctness] Truncate before normalization
Truncate before normalization
`BracketedPattern.authNofMth` now truncates the raw family name after a regex replace, before later key normalization expands characters (e.g., ö→oe), so patterns like `[auth3]` / `[auth4_1]` can yield longer/different prefixes than intended. This contradicts existing expectations (e.g., tests expecting `Köning` with `[auth3]` to produce `Koe`) and can lead to unexpected key changes/collisions.BracketedPattern.authNofMth truncates the author family name before the citation-key normalization step that expands some Unicode characters (e.g., ö → oe). This changes the semantics of [authN] / [authN_M] / [authIniN] because truncation is done on the pre-normalized string, but normalization happens later and can increase length or alter the prefix.
-
BracketedPattern.getFieldValueroutesauth\d+andauth\d+_\d+patterns toauthNofMth, which performs the truncation. -
CitationKeyGenerator.expandBracketContentsubsequently callsremoveUnwantedCharacters(...), which invokesStringUtil.replaceSpecialCharacters(...)usingUnicodeToReadableCharMap(e.g.,\u00F6→oe). - Because there is no re-truncation after normalization, the final output can exceed the intended
Ncharacters and differ from prior behavior.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java[960-978]
- Ensure the same normalization used for citation keys (at minimum
StringUtil.replaceSpecialCharacters, and ideally the full unwanted/disallowed filtering logic) is applied before takingsubstring(0, n)inauthNofMth. - Suggested implementation approach:
- Replace the
replaceAll(...)with a call that normalizes first, then removes unwanted/disallowed characters, then truncates. - Prefer reusing existing logic (e.g., extract a shared helper for
removeUnwantedCharacters-style normalization) rather than hardcoding a regex character class. - Add/adjust a focused unit test for
[auth3]/[auth4_1]with an umlauted name (e.g.,Köning,Gödel) to lock in the intended behavior (truncate after normalization).
[maintainability] Unwanted chars duplicated in regex
Unwanted chars duplicated in regex
The unwanted-characters set is hard-coded inside a regex literal in `authNofMth`, duplicating the canonical default unwanted-characters definition. This duplication risks future drift/inconsistency if defaults change.BracketedPattern.authNofMth embeds the unwanted-characters list directly in the regex literal ([-ʹ:!;?^$]*`). This duplicates the default unwanted-characters definition maintained elsewhere.
Duplicated business rules (like "which characters are unwanted") can easily diverge over time and make bug fixes incomplete.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java[974-976]
[performance] `cleanKey()` uses `replaceAll`
`cleanKey()` uses `replaceAll`
`cleanKey` calls `replaceAll("\\s", "")`, which recompiles the regex on every invocation and violates the compiled-regex requirement for non-trivial/repeated regex usage. This can add avoidable overhead in citation-key generation paths.CitationKeyGenerator.cleanKey currently removes whitespace via String.replaceAll("\\s", ""), which recompiles the pattern per call.
The compliance checklist requires using compiled Pattern instances for repeated/non-trivial regex operations.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyGenerator.java[151-152]
[maintainability] Default constant mis-layered
Default constant mis-layered
CitationKeyPatternPreferences now sources its default unwanted-characters value from CitationKeyGenerator.DEFAULT_UNWANTED_CHARACTERS, even though CitationKeyGenerator documents that this constant should not be used directly. This increases coupling (preferences depend on generator) and contradicts the documented contract, making future refactors riskier.CitationKeyPatternPreferences now depends on CitationKeyGenerator.DEFAULT_UNWANTED_CHARACTERS for its default value, while CitationKeyGenerator explicitly says the constant should not be used directly. This creates an unnecessary dependency from the preferences layer to the generator layer and contradicts the comment.
The PR goal is to avoid duplicated defaults, but the current direction of dependency makes CitationKeyPatternPreferences require the generator class to define its own defaults.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyPatternPreferences.java[59-74]
- jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyGenerator.java[28-36]
- Move the default unwanted-characters constant into
CitationKeyPatternPreferences(or a small dedicatedCitationKeyDefaults/CitationKeyConstantsclass in the same package). - Update
CitationKeyGeneratorto reference that constant (or remove/rename the misleading comment if the generator is intended to be the source-of-truth). - Keep only one source-of-truth, but ensure dependencies flow from preferences/constants -> generator, not the reverse.
| PR 15732 (2026-05-13) |
[maintainability] Extra blank line in changelog
Extra blank line in changelog
An extra blank line was added at the end of `CHANGELOG.md`, which violates the changelog formatting rule to not introduce additional blank lines. This can accumulate and reduce changelog readability/consistency.CHANGELOG.md has an added blank line near the end of the file.
The changelog rules require not adding extra blank lines.
- CHANGELOG.md[2035-2035]
[correctness] Stale focus restore state
Stale focus restore state
EntryEditor captures lastFocusedFieldName on Alt+UP/DOWN, but if navigation does not change the edited entry, setCurrentlyEditedEntry can return early and never clears this state. A later entry change can then unexpectedly force focus to a previously focused field.lastFocusedFieldName is set before navigating to next/previous entry, but it is only cleared inside the focus-restore block in setCurrentlyEditedEntry. If entry navigation does not change the edited entry (common at first/last entry), setCurrentlyEditedEntry may return early and never clear lastFocusedFieldName, so the next real entry change can restore focus to a stale/incorrect field.
-
captureFocusedField()setslastFocusedFieldNameright before callingselectNextEntry()/selectPreviousEntry(). -
setCurrentlyEditedEntry()has an early-return when the entry instance is unchanged, which bypasses the clearing logic. -
LibraryTab.selectNextEntry/selectPreviousEntrymove by index +/- 1 without explicit bounds checks, so it is possible for selection not to move at table bounds.
- jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[322-328]
- jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[493-496]
- jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[535-540]
- Ensure
lastFocusedFieldNameis cleared whensetCurrentlyEditedEntryexits early due toObjects.equals(this.currentlyEditedEntry, currentlyEditedEntry). - Optionally, also set
lastFocusedFieldName = nullat the start ofcaptureFocusedField()so a failed capture can’t reuse an older value.
[maintainability] Trivial `///` focus comment
Trivial `///` focus comment
The new `captureFocusedField` method uses a non-standard `///` comment and the added comments restate what the code already expresses rather than explaining rationale. This adds noise and does not follow the project’s guidance to avoid trivial comments and adhere to code style.New comments are trivial (what-not-why) and one uses a non-standard /// comment marker.
Project compliance requires following code style and avoiding comments that merely restate the code.
- jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[322-327]
- jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[535-540]
| PR 15728 (2026-05-12) |
[maintainability] Optional `get()` after `isEmpty()`
Optional `get()` after `isEmpty()`
`CSLCitationRanges` uses `if (range.isEmpty()) continue;` followed by multiple `range.get()` calls, which is non-idiomatic Optional control flow and risks future unsafe access if the code changes.New code uses Optional.isEmpty() and then calls Optional.get() multiple times. This is discouraged; prefer ifPresent, map, or unwrapping once to a local variable.
This occurs in the newly added CSLCitationRanges method while retrieving a reference mark anchor.
- jablib/src/main/java/org/jabref/logic/openoffice/frontend/OOFrontend.java[302-312]
[correctness] Protects non-JabRef marks
Protects non-JabRef marks
OOFrontend.CSLCitationRanges marks every LibreOffice reference mark as a protected citation range, so inserting a CSL citation can fail when the cursor is inside an unrelated reference mark (user-created cross-references/bookmarks or other extensions’ marks). This can create false “cursor is in a protected area” errors and prevent valid citation insertion.CSLCitationRanges currently iterates over all LibreOffice reference marks (UnoReferenceMark.getListOfNames) and treats them as citation-protected ranges. This can block citation insertion whenever the cursor happens to be inside any unrelated reference mark.
Other parts of the codebase already filter reference marks to JabRef-owned ones (both for JStyle and CSL flows). CSLCitationRanges should similarly only include JabRef CSL citation reference marks (and optionally also JabRef JStyle marks if you want protection across style types).
- jablib/src/main/java/org/jabref/logic/openoffice/frontend/OOFrontend.java[295-316]
- Filter
UnoReferenceMark.getListOfNames(doc)down to JabRef CSL citation marks only (e.g., using the same token/prefix logic asCSLReferenceMarkManager.readAndUpdateExistingMarks: split by space, requireparts.length >= 3,parts[0].startsWith(ReferenceMark.PREFIXES[0])andparts[1].startsWith(ReferenceMark.PREFIXES[1])). - Keep skipping marks with missing anchors.
- (Optional hardening) If you want cursor protection to work even in mixed documents, build the protected list as (JStyleCitationRanges + filtered CSLCitationRanges) instead of relying on unfiltered “all reference marks” behavior.
[maintainability] Uppercase method names added
Uppercase method names added
The new methods `JStyleCitationRanges` and `CSLCitationRanges` start with an uppercase letter, which violates Java/JabRef lowerCamelCase naming conventions and reduces readability/consistency.Newly introduced method names use UpperCamelCase (JStyleCitationRanges, CSLCitationRanges) instead of Java/JabRef lowerCamelCase, reducing codebase consistency.
These methods are new/modified in the OpenOffice frontend logic and should follow JabRef naming conventions.
- jablib/src/main/java/org/jabref/logic/openoffice/frontend/OOFrontend.java[275-316]
[maintainability] Changelog entry is unpolished
Changelog entry is unpolished
The new CHANGELOG entry contains grammatical issues (e.g., `another citations`, `which break`) and reads unprofessionally for end-user documentation.The added CHANGELOG entry has grammatical errors and should be rewritten to be professional and user-facing.
CHANGELOG entries are user-facing documentation and should be polished and consistent.
- CHANGELOG.md[59-59]
| PR 15712 (2026-05-11) |
[maintainability] `pdfPath` uses `isPresent`+`get`
`pdfPath` uses `isPresent`+`get`
The new OCR action uses `if (!pdfPath.isPresent())` followed by `pdfPath.get()`, which is the non-idiomatic and error-prone Optional pattern the project forbids. This reduces readability and can lead to unsafe refactors later.Optional is handled using isPresent() + get(), which violates the project's Optional usage rules.
In OcrLinkedFileAction.execute(), the code checks presence and then calls get() to pass the Path into the OCR task.
- jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[51-56]
[correctness] Missing OCR l10n keys
Missing OCR l10n keys
New user-facing OCR UI strings are passed to `Localization.lang(...)` in `OcrLinkedFileAction`, but their translation keys are missing from `JabRef_en.properties`, so they cannot be translated. Because missing keys are silently accepted, this undermines localization completeness and consistency for the new OCR flow, including error paths.Two new user-facing OCR messages are looked up via Localization.lang(...) but are not present in JabRef_en.properties, preventing translation and allowing missing keys to slip through silently.
OcrLinkedFileAction shows an error dialog when no file is found and a notification on unexpected failure, both using new localization keys/strings that are not defined in the English resource bundle. JabRef’s localization bundle behavior returns the key itself for missing entries and does not reliably warn, making incomplete localization coverage for these OCR error paths easy to miss.
- jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[50-88]
- jablib/src/main/resources/l10n/JabRef_en.properties[797-808]
- jablib/src/main/java/org/jabref/logic/l10n/Localization.java[130-168]
[correctness] OCR adds untyped file
OCR adds untyped file
OcrLinkedFileAction adds the OCR output using `new LinkedFile(Path)` which creates a linked file with an empty file type and without the normal path relativization/type-guessing used elsewhere. This prevents the OCR output from being recognized as an offline PDF in the UI and reduces library portability (absolute paths).On OCR success, the code creates new LinkedFile(success.outputFile()), which sets an empty file type and likely stores an absolute path. JabRef has established helpers to guess file type and relativize paths against configured file directories.
LinkedFile(Path) explicitly sets fileType to empty. LinkedFileViewModel treats files as “offline PDF” only when fileType == pdf, so the newly added OCR file won’t behave like a PDF link. LinkedFilesEditorViewModel#fromFile shows the expected pattern: guess type by extension and relativize path.
- jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[60-67]
- jablib/src/main/java/org/jabref/model/entry/LinkedFile.java[96-100]
- jabgui/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java[100-110]
- jabgui/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java[106-108]
| PR 15708 (2026-05-09) |
[maintainability] ADR missing Jekyll front matter
ADR missing Jekyll front matter
The new ADR file does not include the standard MADR/Jekyll front matter (`---`, `nav_order`, `parent`) used by existing decision records, so it likely won’t be indexed/rendered correctly in the documentation site. This does not meet the requirement to create the ADR using the project template/required sections.The new ADR docs/decisions/0056-OCR-engine-selection.md is missing the standard YAML front matter (nav_order, parent) used by JabRef decision records and the provided ADR template.
Without the standard front matter, the ADR may not show up in the rendered Decision Records navigation/index and does not follow the project’s MADR/template conventions required by compliance.
- docs/decisions/0056-OCR-engine-selection.md[1-5]
[maintainability] ADR has spelling/grammar errors
ADR has spelling/grammar errors
The added ADR contains clear spelling/grammar issues (e.g., `eauations`, double punctuation), reducing the professionalism and polish of user-facing documentation. This violates the requirement to keep documentation polished and professional.The ADR text contains spelling/grammar/punctuation mistakes (e.g., mathematical eauations and weights..).
This is user-facing documentation under docs/, and should be polished and professional.
- docs/decisions/0056-OCR-engine-selection.md[15-18]
- docs/decisions/0056-OCR-engine-selection.md[182-185]
| PR 15707 (2026-05-08) |
[maintainability] Misleading test name
Misleading test name
The method name aPACitation2Authors suggests it tests “two authors”, but it actually tests citing two different entries in one combined citation. This can mislead future maintenance and weaken understanding of coverage.aPACitation2Authors is named like it validates a two-author scenario, but the test actually validates a combined in-text citation for two entries.
The method calls generateCitation(List.of(testEntry, testEntry2), ...) and asserts the combined output.
- jablib/src/test/java/org/jabref/logic/citationstyle/CitationStyleGeneratorTest.java[75-84]
Rename the test to something like aPACitationMultipleEntries() or aPACitationTwoEntries() (or similar) so the name matches what is asserted.
| PR 15688 (2026-05-06) |
[maintainability] `Reset current template` unlocalized
`Reset current template` unlocalized
The new/modified FXML introduces a hard-coded user-facing string (`Reset current template`) instead of using a localization key. This breaks localization and UI text consistency across languages.AiTab.fxml contains a hard-coded user-facing button label (Reset current template) instead of a %... localization key.
Project UI strings must be localized (FXML text values should be %key).
- jabgui/src/main/resources/org/jabref/gui/preferences/ai/AiTab.fxml[345-347]
[reliability] `extractCitationsUsingLLM` catches `Exception`
`extractCitationsUsingLLM` catches `Exception`
`extractCitationsUsingLLM(...)` catches generic `Exception` and wraps it in a new `RuntimeException`, which can hide expected failure modes and crash callers unexpectedly. This violates the requirement for specific, minimal exception handling and avoiding unchecked exception wrapping.The method extractCitationsUsingLLM catches Exception broadly and rethrows new RuntimeException(e), which is against the exception-handling policy.
Prefer catching only expected exception types (and handle/log appropriately) rather than Exception, and avoid introducing unchecked exception wrapping in normal flows.
- jablib/src/main/java/org/jabref/logic/importer/fileformat/pdf/CitationsFromPdf.java[27-38]
[correctness] Summary tasks not deduped
Summary tasks not deduped
`SummarizationTaskAggregator.startNewTask(...)` never stores the created `GenerateSummaryTask` in `tasks`, so `start()` always creates a fresh task for the same entry. This breaks deduplication and can trigger duplicate summarization requests and inconsistent UI/task state.SummarizationTaskAggregator is intended to deduplicate summarization tasks, but startNewTask(...) never inserts the created task into the tasks map. As a result, getTask(...) always returns empty and deduplication never happens.
The aggregator maintains tasks keyed by BibEntry (by id comparator), but the map is only read and removed from—never written.
- jablib/src/main/java/org/jabref/logic/ai/summarization/SummarizationTaskAggregator.java[46-85]
- In
startNewTask(...), addtasks.put(request.fullEntry().entry(), task);before executing/returning. - Consider simplifying
start(...)to a singlecomputeIfAbsentflow and ensureonFinishedremoval matches the same key used for insertion.
[correctness] `getKind()` returns `null`
`getKind()` returns `null`
`AverageTokenEstimator`, `ByCharacterTokenEstimator`, and `ByWordsTokenEstimator` return `null` from `getKind()`, which breaks the non-null contract of the API and can cause NPEs where the kind is used. These implementations should return the corresponding `TokenEstimatorKind` enum constants.Several TokenEstimator implementations return null from getKind(), violating null-safety expectations and risking NPEs.
TokenEstimatorKind defines AVERAGE, WORDS, and CHARS, so the concrete estimators should return a non-null kind.
- jablib/src/main/java/org/jabref/logic/ai/tokenization/logic/AverageTokenEstimator.java[32-35]
- jablib/src/main/java/org/jabref/logic/ai/tokenization/logic/ByCharacterTokenEstimator.java[31-34]
- jablib/src/main/java/org/jabref/logic/ai/tokenization/logic/ByWordsTokenEstimator.java[31-34]
[reliability] `FileHasher` throws `RuntimeException`
`FileHasher` throws `RuntimeException`
`FileHasher.computeHash(...)` logs `NoSuchAlgorithmException` but then throws a new `RuntimeException`, contradicting the method’s `Optional`-based error signaling and risking crashes in new ingestion flows. Prefer returning `Optional.empty()` (or another non-unchecked failure strategy) consistently.FileHasher.computeHash throws RuntimeException on NoSuchAlgorithmException, despite returning Optional<String>.
Compliance requires avoiding unchecked exceptions in new code where possible, and the method already models failure via Optional.empty().
- jablib/src/main/java/org/jabref/logic/ai/ingestion/util/FileHasher.java[41-46]
[correctness] RAG assert can crash
RAG assert can crash
GenerateRagResponseTask.call() relies on an `assert` to guarantee a last USER message exists, then unconditionally calls `userMessage.get()`, which throws when the history contains no USER message (asserts are typically disabled in production). This can crash AI reply generation.GenerateRagResponseTask.call() uses assert userMessage.isPresent() and then calls userMessage.get(). In production JVMs assertions are usually disabled, so an empty chat history (or a history without USER messages) will throw NoSuchElementException and fail the task.
ChatHistoryUtils.getLastUserMessage(...) explicitly returns Optional.empty() when no USER message exists.
- jablib/src/main/java/org/jabref/logic/ai/chatting/tasks/GenerateRagResponseTask.java[47-57]
- jablib/src/main/java/org/jabref/logic/ai/chatting/util/ChatHistoryUtils.java[74-85]
Replace the assert with explicit handling, e.g.:
ChatMessage user = ChatHistoryUtils.getLastUserMessage(...).orElseThrow(() -> new IllegalStateException("No user message to answer"));- or return a
ChatMessage.errorMessage(...)/ fail the task with a user-friendly error, depending on your task/UI contract.
[maintainability] FXML texts end with colons
FXML texts end with colons
New UI texts in FXML end with `:`, which violates JabRef’s UI wording/localization style and can look inconsistent in the UI. This should be rephrased to remove trailing colons.Several newly added FXML UI texts end with a trailing colon (:), which is disallowed by the UI label style rule.
The UI currently introduces strings like %Follow-up questions: and %Summarization algorithm:. JabRef requires labels/texts not to end with :.
- jabgui/src/main/resources/org/jabref/gui/ai/chat/AiChat.fxml[80-80]
- jabgui/src/main/resources/org/jabref/gui/ai/summary/AiSummaryParameters.fxml[15-15]
- jabgui/src/main/resources/org/jabref/gui/ai/summary/AiSummary.fxml[26-26]
[reliability] `AiService` catches generic Exception
`AiService` catches generic Exception
The new migration wrapper catches `Exception`, which is overly broad and can hide programming errors or unexpected runtime failures. Catch only the specific expected exception types (and/or handle them explicitly).AiService.migrateDatabase uses catch (Exception e), which is overly broad.
The compliance rule requires catching specific exceptions and logging them properly. While the logging call is correct, the catch type is too general and may swallow unexpected exceptions.
- jablib/src/main/java/org/jabref/logic/ai/AiService.java[176-191]
[maintainability] Changelog entry is implementation-focused
Changelog entry is implementation-focused
The CHANGELOG entry describes an internal refactoring rather than a user-visible change. Changelog entries must be end-user oriented.A new CHANGELOG entry is written from an implementation perspective (refactoring) instead of describing user impact.
Changelog entries should explain what changed for users, not how the code was changed internally.
- CHANGELOG.md[39-39]
[correctness] Cancelled task becomes success
Cancelled task becomes success
TrackedBackgroundTask.call() unconditionally sets status SUCCESS after perform() returns and sets ERROR for any exception, so a cancelled task can end up reported as SUCCESS/ERROR instead of CANCELLED. Code that relies on CANCELLED (e.g., summarization regenerate path) can misbehave and the UI can display the wrong task state.TrackedBackgroundTask.cancel() sets status to CANCELLED, but call() later overwrites status to SUCCESS after perform() completes, and overwrites to ERROR for any exception (including interruption/cancellation). This breaks logic that checks for CANCELLED and can show incorrect status in UI.
SummarizationTaskAggregator.start() checks task.getStatus() == CANCELLED to decide whether it may start a new regeneration task.
- jablib/src/main/java/org/jabref/logic/ai/util/TrackedBackgroundTask.java[38-80]
- jablib/src/main/java/org/jabref/logic/ai/summarization/SummarizationTaskAggregator.java[53-56]
In call(), before setting SUCCESS, check isCancelled() / Thread.currentThread().isInterrupted() and keep/set CANCELLED. In the catch block, treat interruption/cancellation distinctly (set CANCELLED rather than ERROR) and avoid overwriting an already-cancelled status.
| PR 15685 (2026-05-05) |
[correctness] Autosave path must exist
Autosave path must exist
When autosave is enabled, folder validation requires Files.exists(Path.of(input)) and the Connect button is gated by this validator, so selecting a new (not-yet-created) .bib file path via the Save dialog will keep Connect disabled. This contradicts the later SaveDatabaseAction.saveAs behavior, which can create a new file at the chosen path.When autosave is enabled, the folderValidator requires that the file path already exists, which blocks selecting a new save target via the Save dialog (common workflow). The code later saves using SaveDatabaseAction.saveAs(Path) which can create the file.
The Connect button is disabled based on formValidation().validProperty(). This PR added folderValidator to formValidator, so the existence check now blocks connecting.
- jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseLoginDialogViewModel.java[137-152]
Change the autosave path predicate to accept a non-empty, syntactically valid path where the parent directory exists and is writable (or at least exists), instead of requiring Files.exists(path).
For example:
- Parse with
Path.of(input)inside atry/catchforInvalidPathException - Validate
path.getParent() != null && Files.isDirectory(path.getParent()) - Optionally validate write permission if desired
- Keep
Files.exists(path)only for the keystore validator (which really must exist).
[maintainability] Unrelated `.idea` code style edit
Unrelated `.idea` code style edit
The PR includes an IDE code-style configuration change unrelated to the functional UI/validation fix, increasing diff noise and risking unintended formatting churn for other contributors. Compliance requires keeping diffs focused and avoiding unrelated formatting/config changes.An unrelated IntelliJ code style configuration line was added, creating non-functional diff noise.
This PR is about enabling/disabling fields in the shared database dialog; IDE formatting configuration changes are not required for that.
- .idea/codeStyles/Project.xml[24-24]
| PR 15678 (2026-05-04) |
[correctness] Missing partial month tests
Missing partial month tests
Month validation behavior was changed, but no regression tests were added to ensure mixed/partial values like `1abc` or `#jan# trailing` are rejected. This risks reintroducing the original bug without automated detection.MonthChecker behavior was tightened, but regression tests for the originally reported partial/mixed inputs are missing.
Compliance requires tests that fail for values that previously passed due to partial regex matching (e.g., 1abc, #jan# trailing).
- jablib/src/test/java/org/jabref/logic/integrity/MonthCheckerTest.java[31-84]
[correctness] Rejects zero-padded months
Rejects zero-padded months
In BibLaTeX mode, `MonthChecker` now rejects month integers with a leading zero (e.g., "01"/"02"/"09"), returning “should be an integer or normalized”. The codebase already parses and produces these values, so integrity checks/validation can start flagging existing entries as invalid after this change.MonthChecker's BibLaTeX integer validation regex only matches 1–12 without a leading zero. This causes values like "01", "02", "09" (which the codebase parses/produces) to be treated as invalid.
Month.parse explicitly supports “01 to 12”, and multiple tests construct entries with StandardField.MONTH set to zero-padded values.
- jablib/src/main/java/org/jabref/logic/integrity/MonthChecker.java[16-20]
- Update the integer regex to accept an optional leading zero for 1–9, e.g.:
^(0?[1-9]|1[0-2])$- Add/extend unit tests (e.g.,
MonthCheckerTest) to cover: - Accept:
01,02,09,10,12 - Reject:
00,13,01abc,#jan# trailing
[maintainability] Changelog uses internal classname
Changelog uses internal classname
The new CHANGELOG entry references the internal class name `MonthChecker` and deviates from the surrounding end-user-focused phrasing style. This reduces readability and consistency for end users.CHANGELOG entry is a bit too implementation-specific and inconsistent in style.
Nearby entries use end-user phrasing (e.g., "We fixed...") and generally avoid internal class names.
- CHANGELOG.md[107-107]
- Home
- General Information
- Development
- Please go to our devdocs at https://devdocs.jabref.org
- Completed "Google Summer of Code" (GSoC) projects
- GSoC 2025 ‐ Git Support for JabRef
- GSoC 2025 - LSP
- GSoC 2025 - Walkthrough and Welcome Tab
- GSoC 2024 ‐ Improved CSL Support (and more LibreOffice‐JabRef integration enhancements)
- GSoC 2024 - Lucene Search Backend Integration
- GSoC 2024 ‐ AI‐Powered Summarization and “Interaction” with Academic Papers
- GSoC 2022 — Implement a Three Way Merge UI for merging BibTeX entries
- GSoC 2021 - Improve pdf support in JabRef
- GSoC 2021 - Microsoft Word Integration
- GSoc 2019 - Bidirectional Integration — Paper Writing — LaTeX and JabRef 5.0
- GSoC Archive
- Release
- JabCon Archive