Skip to content

.pr_agent_accepted_suggestions

Jump to bottom
qodo-merge-bot edited this page Jun 11, 2026 · 225 revisions
                     PR 15938 (2026-06-09)                    
[maintainability] Hardcoded border color
Hardcoded border color The new `.bordered` CSS class hardcodes `-fx-border-color: black`, which is not theme-aware and can look wrong in dark/custom themes compared to the rest of the theme that uses variables (e.g., `-fx-outer-border`, `-jr-separator`). This undermines the intent of moving styling into theme CSS with consistent, overridable tokens.

Issue description

The .bordered class introduced in jabref-theme.css hardcodes black as border color. This is inconsistent with the rest of JabRef’s theming approach (which relies heavily on CSS variables like -fx-outer-border / -jr-*) and may reduce contrast or visual consistency in dark/custom themes.

Issue Context

JabRef already defines theme variables and uses them widely for borders/backgrounds. A generic utility class like .bordered should follow the same convention.

Fix Focus Areas

  • jabgui/src/main/resources/org/jabref/gui/jabref-theme.css[70-86]

Suggested fix

Change .bordered to use a theme token, e.g.:

  • -fx-border-color: -fx-outer-border; (matches existing usage) or
  • -fx-border-color: -jr-separator; / another existing -jr-* border token if that better matches the design system. Keep -fx-border-width: 1; unchanged unless a token exists for width.
[maintainability] Misnamed padding class
Misnamed padding class The new CSS class name `.padding-left-medium` contradicts the padding it applies (`-fx-padding: 0 15 0 0`), which is used elsewhere in the theme file as “right padding” based on the same 4-value padding convention. This naming mismatch makes the class easy to misuse and increases the chance of layout mistakes when reused.

Issue description

.padding-left-medium is defined with -fx-padding: 0 15 0 0, which (by the same convention used elsewhere in JabRef CSS) represents right padding, not left padding. The class name is therefore misleading.

Issue Context

The class is already referenced from NewEntry.fxml for the idJumpLink hyperlink, so the name will be encountered and likely reused.

Fix Focus Areas

  • jabgui/src/main/resources/org/jabref/gui/jabref-theme.css[83-85]
  • jabgui/src/main/resources/org/jabref/gui/newentry/NewEntry.fxml[113-119]

Suggested fix

Pick one:

  1. If the intent is right padding (matches the current values), rename to something like .padding-right-medium (or .padding-right-15) and update NewEntry.fxml accordingly.
  2. If the intent is left padding, keep the name but change the values to -fx-padding: 0 0 0 15; and verify the UI still looks correct.
                     PR 15921 (2026-06-08)                    
[reliability] `prefs.get(..., null)` in migration
`prefs.get(..., null)` in migration The new migration code passes `null` as the default value to `prefs.get(...)` to test presence, which violates the project’s preference to avoid passing/using `null` when alternatives exist. This can be replaced with `prefs.hasKey(...)` and/or `prefs.getAsOptional(...)` to keep nullability explicit and safer.

Issue description

PreferencesMigrations.upgradeImportFileAndDirePatterns and migrateFileImportPattern call prefs.get(..., null) which passes null as an argument. The compliance checklist requires avoiding passing/using null where reasonable.

Issue Context

JabRefCliPreferences provides non-null alternatives such as hasKey(String) and getAsOptional(String).

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/migrations/PreferencesMigrations.java[205-231]
                     PR 15916 (2026-06-07)                    
[maintainability] Typo in test method name
Typo in test method name The newly added test method name `unmigatedFetcherThrowsOnRawQuery` contains a spelling error (`unmigated`), which reduces readability and makes the test harder to search for consistently. This violates the requirement for meaningful names without typos and should be corrected since it’s newly introduced.

Issue description

The newly added test method name is misspelled: unmigatedFetcherThrowsOnRawQuery should be renamed to unmigratedFetcherThrowsOnRawQuery to meet the requirement for correctly spelled, meaningful method names.

Issue Context

Correct spelling improves readability, maintainability, and searchability of tests, and this is a minor issue that is easy to fix as part of the same PR (including updating any references if present).

Fix Focus Areas

  • jablib/src/test/java/org/jabref/logic/importer/SearchBasedFetcherTest.java[101-106]
                     PR 15914 (2026-06-07)                    
[maintainability] Missing requirement for hover preview
Missing requirement for hover preview This PR adds a new user-facing feature (citation preview tooltip on hover) but does not add a corresponding OpenFastTrace requirement under `docs/requirements/`. This breaks requirements tracing expectations for new features and reduces long-term maintainability/auditability.

Issue description

A new feature (citation preview tooltip on hover in the Entry Editor's "Citations" tab) was introduced without adding a corresponding OpenFastTrace requirement in docs/requirements/<area>.md.

Issue Context

The PR adds hover-based citation preview behavior in the GUI and documents it in the changelog, which indicates this is a user-visible feature that should be traceable via an OpenFastTrace requirement.

Fix Focus Areas

  • docs/requirements/entry-editor.md[4-15]
  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[583-589]
  • CHANGELOG.md[14-14]
[performance] Eager `orElse(new BibDatabaseContext())`
Eager `orElse(new BibDatabaseContext())` The new hover handler uses `stateManager.getActiveDatabase().orElse(new BibDatabaseContext())`, which eagerly allocates a `BibDatabaseContext` on every hover even when an active database is present. This is non-idiomatic Optional usage on a hot UI path and can add unnecessary work/GC pressure when moving across many items.

Issue description

The hover handler currently calls stateManager.getActiveDatabase().orElse(new BibDatabaseContext()), which eagerly allocates a new BibDatabaseContext every time the handler runs, even when the Optional is present, creating unnecessary allocations on a frequent onMouseEntered UI path.

Issue Context

This code executes on onMouseEntered for each rendered citation item, making it a hot path where repeated allocations can contribute to GC pressure/jank. The compliance checklist (PR Compliance ID 8) requires fluent Optional handling and avoiding orElse(default) fallbacks that are never (or almost never) needed.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[594-598]
[maintainability] Duplicated PreviewViewer tooltip setup
Duplicated PreviewViewer tooltip setup The PR adds new tooltip initialization logic (creating `PreviewViewer`, calling `resizeForTooltipContent()`, and listening for `Worker.State.SUCCEEDED` to resize the tooltip) that duplicates the existing preview-tooltip pattern already implemented elsewhere. This increases maintenance cost and risks divergence of tooltip behavior across the UI.

Issue description

Tooltip/preview initialization logic is duplicated instead of reusing a shared component.

Issue Context

There is already an existing preview-tooltip implementation (MainTableTooltip) that sets up a PreviewViewer for tooltips and performs sizeToScene() on successful web-engine load. The PR reintroduces a very similar setup inside CitationRelationsTab.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[180-188]
  • jabgui/src/main/java/org/jabref/gui/maintable/MainTableTooltip.java[16-33]
[reliability] Stale preview race
Stale preview race Hovering triggers asynchronous preview generation for each entered item; if the mouse moves quickly between items, earlier background tasks can finish later and overwrite the tooltip with a preview for the wrong entry. This can show incorrect citation previews and wastes background work even when the tooltip never ends up being shown.

Issue description

CitationRelationsTab updates a shared PreviewViewer on onMouseEntered, which triggers async preview generation. PreviewViewer.update() applies results without verifying the entry is still current, so out-of-order task completion can display stale content.

Issue Context

PreviewViewer.update() captures currentEntry but calls setPreviewText(previewText) unconditionally in onSuccess/onFailure. It only guards against staleness in the cover-download refresh path.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[594-600]
  • jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[200-221]

Suggested change

Option A (recommended):

  • In PreviewViewer.update(), introduce a monotonically increasing request id (or capture both entry and databaseContext) and in onSuccess/onFailure only apply results if the captured values still equal the current this.entry and this.databaseContext. Option B (mitigation):
  • Move the setLayout/setDatabaseContext/setEntry calls from onMouseEntered to a tooltip lifecycle hook (e.g., previewTooltip.setOnShowing(...)) and/or debounce, to reduce the number of background tasks started while the user is just moving the cursor.
                     PR 15913 (2026-06-07)                    
[maintainability] `ExportService` prints hardcoded text
`ExportService` prints hardcoded text `ExportService` and the new Picocli options (`--output`, `--output-format`) emit hardcoded, non-localized English user-facing text, and `ExportService` also constructs a localized warning via string concatenation. This prevents translation and breaks placeholder-based localization formatting required for CLI output and help text.

Issue description

ExportService prints user-facing messages that are not localized and builds localized output by concatenating strings, and the Picocli option help texts for --output and --output-format are hardcoded in English.

Issue Context

Compliance requires all user-facing CLI text (including Picocli help/usage output) to go through Localization.lang(...) and to avoid constructing localized messages by concatenating strings; instead, define a single localized message with placeholders and store the English defaults in jablib/src/main/resources/l10n/JabRef_en.properties.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[127-129]
  • jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[142-146]
  • jabkit/src/main/java/org/jabref/toolkit/commands/GetCitedWorks.java[37-48]
  • jabkit/src/main/java/org/jabref/toolkit/commands/GetCitingWorks.java[37-48]
  • jablib/src/main/resources/l10n/JabRef_en.properties[328-334]
[maintainability] `ExportServiceTest` catches exceptions
`ExportServiceTest` catches exceptions The new test catches `ExportServiceException` via a manual try/catch block instead of using JUnit’s `assertThrows`. This violates the project’s test conventions and can reduce clarity of test intent.

Issue description

A unit test catches exceptions with a try/catch instead of using assertThrows.

Issue Context

Project test conventions require not catching exceptions in tests; JUnit should handle them and assertions should be expressed via assertThrows.

Fix Focus Areas

  • jabkit/src/test/java/org/jabref/toolkit/service/ExportServiceTest.java[58-72]
[correctness] Check ignores findings code
Check ignores findings code `Check.checkConsistency()` and `Check.checkIntegrity()` ignore the `int` returned by `CheckConsistency.execute(...)` / `CheckIntegrity.execute(...)` and always return `ExitCode.OK` when no exception is thrown, so `jabkit check FILE` can exit 0 even when findings exist (which should exit 1).

Issue description

Check.checkConsistency() and Check.checkIntegrity() currently discard the return value of the underlying execute(...) methods and always return ExitCode.OK on success. This breaks the CLI contract that jabkit check <file> should exit with 1 when findings exist (and 0 only when clean).

Issue Context

Both CheckConsistency.execute(...) and CheckIntegrity.execute(...) still return int exit codes indicating findings vs. clean; the wrapper methods must propagate those values.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[61-79]
[correctness] Export logs break porcelain
Export logs break porcelain `ExportService.tryExportWithExporter(...)` always prints an "Exporting …" line, which (a) produces duplicate output because `exportParserResultToFile(...)` already prints, and (b) emits status text even when callers request `--porcelain` output.

Issue description

ExportService.tryExportWithExporter(...) prints to stdout unconditionally. This causes:

  • Duplicate "Exporting" messages (one in exportParserResultToFile, one in tryExportWithExporter).
  • --porcelain output to be polluted with status text (e.g., Convert passes sharedOptions.porcelain, but it is not respected).

Issue Context

Porcelain is documented as "script-friendly output" and should avoid non-data status lines.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[173-211]
  • jabkit/src/main/java/org/jabref/toolkit/commands/Convert.java[53-64]
[correctness] BibTeX exporter ignores entries
BibTeX exporter ignores entries The custom `bibtexExporter` ignores the `entries` parameter and always saves the full `BibDatabaseContext`, so any call site that exports a subset via `exportBibDatabaseContextToFile(context, subset, ...)` can accidentally export more entries than requested.

Issue description

The custom BibTeX exporter implementation ignores the entries list passed to Exporter.export(...). This violates the exporter contract (export the provided subset) and can lead to exporting unintended entries when a BibDatabaseContext contains more entries than the subset list.

Issue Context

The Exporter API explicitly passes List<BibEntry> entries to allow subset exporting.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[61-67]
  • jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[151-171]
                     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.

Issue description

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.

Issue Context

  • clear() and importPreferences() call getImporterPreferences().setAll(...).
  • getApiKeys() changes trigger keyring store/clear.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/importer/ImporterPreferences.java[118-130] Suggested fix: In setAll, set persistCustomKeys before calling setApiKeys(...) (matching the ordering already documented/used in WebSearchTabViewModel.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.

Issue description

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.

Issue Context

  • hasKey checks only an exact key in the backing store.
  • getSeries(prefix) reads prefix + N.
  • storeCustomImportFormats writes prefix + i.

Fix Focus Areas

  • 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]

Suggested fix

Replace the guard with a check that matches the storage format, e.g.:

  • if (!hasKey(IMPORTER_CUSTOM_FORMAT + "0")) return defaults; or
  • remove the hasKey guard 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).

Issue description

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.

Issue Context

  • getImporterPreferences() passes ImporterPreferences.getDefault() into getImporterPreferencesFromBackingStore(...).
  • getStringList(key) uses get(key) internally; if the key is absent, it can yield an empty list.
  • JabRef GUI preferences inherit this behavior because JabRefGuiPreferences extends JabRefCliPreferences.

Fix Focus Areas

  • 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]

Suggested fix

In getImporterPreferencesFromBackingStore:

  • Compute catalogs with a fallback:
  • List<String> catalogs = getStringList(IMPORTER_CATALOGS);
  • if (catalogs.isEmpty()) { catalogs = defaults.getCatalogs(); }
  • pass catalogs into the ImporterPreferences constructor. Alternative: introduce a helper getStringList(key, List<String> defaultList) that uses get(key, convertListToString(defaultList)).
                     PR 15899 (2026-06-03)                    
[correctness] Artifact permissions too restrictive
Artifact permissions too restrictive The workflow sets `permissions` to only `contents: read` but uses `actions/upload-artifact` and `actions/download-artifact`, which can fail when the Actions permission scope isn’t granted. This can prevent the smoke-test job from receiving the built binary and make the workflow non-functional.

Issue description

The workflow restricts GITHUB_TOKEN permissions to contents: read, but it uploads and downloads artifacts. Artifact APIs may require actions permission, so the workflow can fail at artifact upload/download.

Issue Context

Other workflows in this repository that upload artifacts grant actions: write, indicating the repo expects explicit actions permissions when artifacts are used.

Fix Focus Areas

  • .github/workflows/jabkit-native-smoke-test.yml[6-8]

Suggested fix

Update the workflow permissions to include at least actions: write (or actions: read + job-level overrides for upload steps), e.g.:

[observability] Smoke test hides stderr
Smoke test hides stderr The clitest smoke test redirects stderr from `jabkit --help` to `/dev/null`, which removes the most useful diagnostics if the native binary fails to start or prints errors on stderr. This makes CI failures harder to debug.

Issue description

The smoke-test command discards stderr (2>/dev/null). When the binary fails to initialize, stderr often contains the actionable failure reason; discarding it makes CI failures opaque.

Issue Context

JabKit prints user-facing error messages to stderr in some failure paths.

Fix Focus Areas

  • jabkit/src/nativeTest/clitest/jabkit.md[1-3]

Suggested fix

Remove the stderr redirection, or capture it to a file and print it on failure. Example:

                     PR 15898 (2026-06-03)                    
[correctness] Git option bypass
Git option bypass The guard only triggers when the raw command contains the literal substring "git commit", so valid commit invocations with git global options in between (e.g., `git -c ... commit ...`) are not checked and will bypass the deny. This leaves a realistic path for the same malformed here-string commit to still be created.

Issue description

commit-msg-heredoc-guard.py uses a raw substring check ("git commit" in command) to decide whether to inspect a command. This misses real git commit invocations when git global options (e.g., -c, -C, --no-pager) appear between git and commit, causing false negatives.

Issue Context

This repo already has a PreToolUse hook (git-rebase-guard.py) that tokenizes the command and locates the git subcommand while skipping global options.

Fix Focus Areas

  • .claude/hooks/commit-msg-heredoc-guard.py[37-45]
  • .claude/hooks/git-rebase-guard.py[18-34]
[reliability] Quote-unaware regex deny
Quote-unaware regex deny The opener regex is applied to the raw command string, so it can match `@'`/`@"` sequences that occur inside quoted arguments (e.g., an `echo "git commit -m @'..."` or a commit message that literally mentions PowerShell here-strings) and deny even though bash will treat them as literal text. This makes the hook prone to false positives compared to the repo’s other hook which tokenizes to avoid matching inside quoted strings.

Issue description

The here-string detection runs regex over the full raw command, without understanding shell quoting. As a result, @' / @" inside quoted literals can still match and cause a deny, even though they are not PowerShell here-string syntax in that context.

Issue Context

git-rebase-guard.py explicitly uses shlex.split(..., posix=True) so matches inside quoted strings/heredoc bodies are not flagged.

Fix Focus Areas

  • .claude/hooks/commit-msg-heredoc-guard.py[40-45]
  • .claude/hooks/git-rebase-guard.py[9-12]
  • .claude/hooks/git-rebase-guard.py[98-103]
                     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.

Issue description

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).

Issue Context

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).

Fix Focus Areas

  • .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.

Issue description

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”.

Issue Context

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

Fix Focus Areas

  • .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.

Issue description

The header comment describing the appended range format is now inaccurate after changing the output to use line N for single-line hunks.

Issue Context

Keeping this comment correct matters because it documents the externally-visible annotation message format that developers see in CI.

Fix Focus Areas

  • .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.

Issue description

.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.

Issue Context

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.

Fix Focus Areas

  • Update parsing to reliably set file for every file-level diff, including deletions (e.g., parse diff --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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • Parse both sides from the hunk header (-oldStart,oldCount and +newStart,newCount).
  • Maintain both oldLine and newLine counters 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).

Issue description

.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"

Issue Context

This repository contains tracked files with spaces in their path, so this is not purely theoretical.

Fix Focus Areas

  • .github/format-diff-annotations.sh[64-71]

What to change

  1. 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.)
  1. Consider resetting fileRaw/file on ^diff --git to avoid accidentally reusing the previous file if a header isn’t recognized.
  2. Optionally, make flush() a no-op if file is empty to avoid emitting invalid annotations.
                     PR 15876 (2026-06-01)                    
[maintainability] Ambiguous modules.properties location
Ambiguous modules.properties location The new step says to add a mapping to `modules.properties` but then instructs to edit `gradle/modules.properties`, and also links to an upstream `modules.properties` file; this mixes three different identifiers and can send contributors to the wrong place. This is likely to cause wasted time or incorrect documentation-driven changes when resolving module mapping issues.

Issue description

The ordered-list item introduces modules.properties without a path, but the instructions refer to gradle/modules.properties and also mention an upstream modules.properties in the Gradlex project. This is ambiguous and can mislead contributors about where they should add the mapping.

Issue Context

  • The repository contains the local mapping file at gradle/modules.properties.
  • The docs also link to the upstream Gradlex mapping file; this should be clearly labeled as upstream.

Fix Focus Areas

  • docs/code-howtos/dependency-management.md[78-85]

Suggested change

  • Update the list item label to explicitly mention the local path, e.g. Add missing mapping to \\gradle/modules.properties\``.
  • Reword the “Furthermore…” sentence to explicitly call out that the link is to the upstream Gradlex mapping file (and optionally change the link text to remove the ... placeholder-like segment).
                     PR 15874 (2026-06-01)                    
[reliability] Empty cleanup jobs crash
Empty cleanup jobs crash `JabRefCliPreferences#getCleanupPreferencesFromBackingStore` can throw when `CLEANUP_JOBS` exists but is blank/empty, because it calls `EnumSet.copyOf(...)` on an empty collected set. This can break preferences loading (and potentially app startup) after users disable all cleanup steps, since the key can remain present with an empty value.

Issue description

getCleanupPreferencesFromBackingStore builds an EnumSet via EnumSet.copyOf(collectedSet) when hasKey(CLEANUP_JOBS) is true. If the stored list is empty/blank, convertStringToList returns an empty list, leading to an empty Set and an exception from EnumSet.copyOf(...).

Issue Context

This is not prevented by the current hasKey(CLEANUP_JOBS) guard, because hasKey returns true even when the stored value is an empty string.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1814-1833]

Suggested fix approach

  • In getCleanupPreferencesFromBackingStore, after parsing the list, if it is empty, return EnumSet.noneOf(CleanupStep.class) (or fall back to defaults) instead of calling EnumSet.copyOf.
  • Optionally also adjust the active-jobs listener: if cleanupPreferences.getActiveJobs() is empty, remove(CLEANUP_JOBS) instead of writing an empty string, to keep hasKey false for the empty case.
[correctness] Empty focused path persisted
Empty focused path persisted `LastFilesOpenedPreferences.getDefault()` uses `Path.of("")` to represent “no last focused file”, but `JabRefCliPreferences` persists any non-null focused path via `toAbsolutePath()`, turning the empty path into the current working directory. Because `clear()` now resets last-opened prefs via `setAll(getDefault())`, a reset can incorrectly write the current directory as `LAST_FOCUSED` and also load it when the key is missing.

Issue description

LastFilesOpenedPreferences uses an empty Path as a sentinel for “no last focused file”. Persistence treats any non-null Path as valid and stores toAbsolutePath(), which converts the empty path to the current working directory; after this PR, clear() calls setAll(LastFilesOpenedPreferences.getDefault()), so reset/clear can persist an unintended focused path.

Issue Context

The persistence layer already uses null as the signal to remove LAST_FOCUSED, so using an empty Path defeats that contract.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/preferences/LastFilesOpenedPreferences.java[23-35]
  • jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[841-879]
  • jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1869-1894]

Suggested fix approach

  • Change LastFilesOpenedPreferences.getDefault() to use null for lastFocusedFile.
  • In getLastFilesOpenedPreferencesFromBackingStore, only create a Path when hasKey(LAST_FOCUSED) and the stored string is non-blank; otherwise set lastFocusedFile to null.
  • In the lastFocusedFileProperty listener, treat blank/empty paths the same as null (remove the key) to avoid persisting a sentinel value.
                     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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • .github/workflows/pr-comment.yml[64-80]

Suggested implementation direction (bash)

  • 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.

Issue description

.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).

Issue Context

Existing Java-based annotation writers escape both properties and data explicitly, including : and , for properties.

Fix Focus Areas

  • .github/format-diff-annotations.sh[19-30]
  • jablib/src/main/java/org/jabref/logic/util/GitHubActionsEscape.java[15-25]

Suggested approach

  • Implement an AWK function equivalent to GitHubActionsEscape.property (escape %, \r, \n, then additionally escape : and ,).
  • Apply it to file before 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).

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • .github/format-diff-annotations.sh[21-29]

Suggested approach

  • Parse $3 (+newStart,newCount) as well as $2.
  • If oldCount > 0, keep current behavior (use old range).
  • Else (pure addition), set start=newStart and end=newStart+newCount-1.
  • Ensure start >= 1 and end >= start before 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • .github/workflows/tests-code.yml[108-129]
  • .github/format-diff-annotations.sh[1-33]

Suggested approach

  • Add optional parameters or env vars to .github/format-diff-annotations.sh (e.g., ANNOTATION_TITLE / ANNOTATION_MESSAGE_PREFIX, or a first arg like format|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).

Issue description

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.

Issue Context

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.

Fix

Ensure customExporters is always backed by a mutable list, e.g.:

  • In ExportPreferences constructor, use FXCollections.observableArrayList(customExporters) or FXCollections.observableList(new ArrayList<>(customExporters)).
  • Keep setCustomExporters/setAll as-is afterward.

Fix Focus Areas

  • 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.

Issue description

Modern Java provides SequencedCollection#getFirst() for readability and consistency; the PR changed first-element access to get(0).

Issue Context

This code previously used getFirst() and the compliance checklist explicitly prefers modern Java 25+ idioms.

Fix Focus Areas

  • 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.

Issue description

A Javadoc/parameter comment references a non-existent or outdated API name, making the documentation misleading.

Issue Context

The code now uses keywordSeparator, but the Javadoc still points to BibEntryProperties#keyWordDelimiterProperty.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyPatternPreferences.java[46-46]
                     PR 15868 (2026-05-31)                    
[security] CLI path check bypass
CLI path check bypass `isLibraryPathAccessAllowed()` returns `true` for `JabRefSrvStateManager` (stand-alone HTTP server), which makes the new `librarypath` validation effectively a no-op in that mode. As a result, any existing local file path can be opened via `librarypath` without being served or user-approved, defeating the intended hardening.

Issue description

In CAYWResource.isLibraryPathAccessAllowed, stand-alone server mode (srvStateManager instanceof JabRefSrvStateManager) is currently treated as automatically allowed. This bypasses the new guard in getBibDatabaseContext and allows arbitrary librarypath access.

Issue Context

JabRefSrvStateManager is the stand-alone HTTP server state manager (no GUI prompt possible). The new hardening should restrict librarypath to libraries that are already being served (via FilesToServe / open databases), otherwise reject.

Fix Focus Areas

  • jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java[227-275]
  • jabsrv/src/main/java/org/jabref/http/JabRefSrvStateManager.java[21-29]
  • jabsrv/src/main/java/org/jabref/http/server/services/ServerUtils.java[55-90]

Concrete fix guidance

  • Change isLibraryPathAccessAllowed so that for JabRefSrvStateManager it returns false (or otherwise never grants access to unserved paths).
  • Ensure the GraphicsEnvironment.isHeadless() rejection logic is actually reachable for headless/stand-alone mode.
  • (Optional) Adjust INVALID_LIBRARY_PATH_ERROR text to reflect “not served / not permitted” if you keep a separate allow path in GUI mode.
                     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.

Issue description

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().

Issue Context

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.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[39-50]
  • jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[107-164]

What to change

  • In setDate(...):
  • Guard against issuedData == null.
  • After issuedData.dateParts non-empty, also validate the first inner list is non-null and non-empty before calling getFirst().
  • Replace direct Integer.parseInt(...) calls with safe parsing (e.g., try/catch NumberFormatException and return/skip setting month/day), so malformed values don’t crash parsing.
  • Consider broadening the parse(...) catch to include RuntimeException (or at least NoSuchElementException/NumberFormatException) and return List.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.

Issue description

The Russian UI translation for See what's new contains a spelling typo (Узнайть).

Issue Context

This string is user-facing and should follow UI text style guidelines (correct spelling).

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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).

Issue Context

JabRefGuiPreferences expects to override getDefaultPath() to use NativeDesktop.get().getDefaultFileChooserDirectory(), but JabRefCliPreferences now defines a private method returning /.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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().

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

StandardField includes standard BibTeX fields like ANNOTE and CHAPTER, but the current csv.begin.layout header line does not include corresponding columns.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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]

Suggested fix

  • Wrap each exported field in a formatter chain that at least applies ReplaceWithEscapedDoubleQuotes (and ideally also replaces \n with 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.

Issue description

GenericCsvExportFormatTest can run in parallel with other exporter tests, but exporter code mutates global static state, which can lead to non-deterministic failures.

Issue Context

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").

Fix Focus Areas

  • 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]

Suggested fix

  • Add the same annotations used by CsvExportFormatTest:
  • @Execution(ExecutionMode.SAME_THREAD)
  • @ResourceLock("exporter")
  • and the required imports.
  • (Optional) Add an @AfterEach cleanup 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".

Issue description

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>).

Issue Context

  • getFileExtension was updated to tolerate InvalidPathException via a fallback, so the invalid-path flow is explicitly supported.
  • getBaseName still returns the raw fileName on InvalidPathException, 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.

Fix Focus Areas

  • 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]

Suggested fix

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 in getFileExtension).

Add/strengthen test

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.

Issue description

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).

Issue Context

  • getBaseName(String) already catches InvalidPathException and 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.

Fix Focus Areas

  • 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]

Suggested fix approach

  • Make getFileExtension(String) resilient by catching InvalidPathException and extracting the “name” portion without Path.of (e.g., FilenameUtils.getName(trimmed)), then proceed with FilenameUtils.getExtension(...).
  • This keeps extension extraction consistent for all callers.
  • Alternatively (less ideal), wrap the getFileExtension(fileName) call inside getValidFileName with 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.of on Windows (e.g., "a:b.pdf") and assert getValidFileName returns 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.

Issue description

The new test constructs repeated-character strings using Stream.generate(...).collect(Collectors.joining()), which is less idiomatic than String#repeat(int) on modern Java.

Issue Context

The project encourages using modern Java (25+) idioms where applicable to improve readability/maintainability.

Fix Focus Areas

  • 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.

Issue description

The new test’s method-chain indentation is inconsistent with the existing formatting in FileUtilTest, creating a noisy/less consistent style.

Issue Context

This PR should preserve established formatting conventions and avoid unnecessary reformatting/stylistic divergence.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

This is an edge case (very long extensions are unusual), but the failure mode is a hard exception in a core utility method.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[172-188]

Suggested fix approach

  • Guard the computed max base length:
  • int maxBaseLen = MAXIMUM_FILE_NAME_LENGTH - extension.map(s -> s.length() + 1).orElse(0);
  • If maxBaseLen <= 0, avoid calling substring with a negative index.
  • Simple safe fallback: return fullCleanedName.substring(0, MAXIMUM_FILE_NAME_LENGTH) when maxBaseLen <= 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.

Issue description

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.

Issue Context

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>.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[39-66]
  • jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[36-72]

Suggested fix approach

  1. Keep file I/O and any string construction on the calling (background) thread.
  2. Precompute the new FILE field string (e.g., via FileFieldWriter.getStringRepresentation(files)) and compare to the current value off-FX.
  3. Only schedule the minimal mutation on the FX thread, e.g. entry.setField(StandardField.FILE, newValue) (or equivalent), and capture the returned FieldChange via an AtomicReference if you need to return it.
  4. Apply the same pattern in both MoveFilesCleanup and RenamePdfCleanup.
                     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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • .github/workflows/tests-code.yml[188-200]

Suggested fix approach

  • 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 ::error lines.
  • 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.

Issue description

The hook assumes tokens[i+1] is the git subcommand, which fails when git global options appear between git and the subcommand.

Issue Context

Git invocations can legally include global options before the subcommand; the hook should identify the actual subcommand before deciding whether to deny.

Fix Focus Areas

  • .claude/hooks/git-rebase-guard.py[18-53]

Implementation notes

  • When you encounter a git token, 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/push checks 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.

Issue description

The git pull check only detects --rebase and --rebase=... tokens, missing other rebase-enabling variants.

Issue Context

To reliably forbid rebase-based pulls, the hook should treat common short/variant flags as equivalent to --rebase.

Fix Focus Areas

  • .claude/hooks/git-rebase-guard.py[35-41]

Implementation notes

  • 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.

Issue description

The push guard only checks for force flags and ignores refspec arguments, so +-prefixed refspec force-updates are not blocked.

Issue Context

To enforce “no force push,” the hook must also inspect non-flag arguments to git push.

Fix Focus Areas

  • .claude/hooks/git-rebase-guard.py[43-52]

Implementation notes

  • 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.

Issue description

Documentation forbids bare git pull, but the enforcement hook only blocks git pull when rebase is explicitly requested.

Issue Context

This mismatch can confuse agents (and humans) about what will be blocked vs. allowed.

Fix Focus Areas

  • AGENTS.md[440-452]
  • .claude/hooks/git-rebase-guard.py[35-41]

Implementation notes

Choose one:

  • Enforce the doc: deny any git pull invocation (regardless of flags) and instruct to use fetch + 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.

Issue description

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.

Issue Context

  • The method is intended (by name and usage) to set the main file directory, but it assigns the provided Path to the wrong field.
  • JabRefCliPreferences.clear() uses FilePreferences.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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

Two test files still return Optional<Path> from getMainFileDirectory(), which should now return Path.

Fix Focus Areas

  • 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]

Suggested fix

  • Replace thenReturn(Optional.of(path)) with thenReturn(path).
  • Remove now-unused Optional imports/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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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]

Suggested fix

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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]

Suggested change

  • Replace the assignment with something explicit and self-documenting, e.g.:
  • tokenCase = !Character.isLowerCase(c);
  • or keep getType but 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

A trailing space at end-of-line violates markdownlint (MD009).

Issue Context

The CI workflow runs markdownlint over docs/**/*.md, and .markdownlint.yml keeps default rules enabled.

Fix Focus Areas

  • docs/requirements/slr.md[13-13]
                     PR 15790 (2026-05-20)                    
[maintainability] Optional `parsed.get()` after check
Optional `parsed.get()` after check `addPlainCitation` checks `parsed.isEmpty()` and then calls `parsed.get()`, which is the non-idiomatic and risk-prone Optional control-flow pattern disallowed by the checklist. This can regress safety/readability and is explicitly called out as a failure case.

Issue description

EntriesResource.addPlainCitation(...) uses Optional.isEmpty() followed by Optional.get(), which is discouraged and listed as a compliance failure case.

Issue Context

The code currently does:

  • if (parsed.isEmpty()) { throw ... }
  • later parsed.get()

Fix Focus Areas

  • jabsrv/src/main/java/org/jabref/http/server/resources/EntriesResource.java[114-126]
[maintainability] Optional `get()` in `findDoi`
Optional `get()` in `findDoi` `findDoi` uses `parsedDoi.isEmpty()` followed by `parsedDoi.get()`, which is the Optional anti-pattern prohibited by the checklist. This reduces readability and can enable unsafe refactors later.

Issue description

RuleBasedPlainCitationParser.findDoi(...) uses Optional.isEmpty() then Optional.get(), which is a prohibited Optional control-flow pattern.

Issue Context

Current code:

  • gets Optional<DOI> parsedDoi = DOI.findInText(input)
  • checks parsedDoi.isEmpty()
  • then reads the value via parsedDoi.get()

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/importer/plaincitation/RuleBasedPlainCitationParser.java[114-121]
[reliability] `LibraryQueryRequest` nullability mismatch
`LibraryQueryRequest` nullability mismatch `LibraryQueryRequest` is `@NullMarked` and declares `queries` as non-null, but the canonical constructor treats `queries` as possibly null (`queries != null ? ...`). This breaks the null-safety contract and should be expressed with explicit nullability annotations or input normalization outside the non-null API surface.

Issue description

Under @NullMarked, record components are non-null by default, but LibraryQueryRequest's constructor treats queries as nullable and normalizes it. This creates a nullability contract mismatch.

Issue Context

The constructor currently does queries = queries != null ? queries : List.of(); even though the record component type is List<String> (non-null under @NullMarked).

Fix Focus Areas

  • jabsrv/src/main/java/org/jabref/http/dto/LibraryQueryRequest.java[17-20]
[correctness] Empty match entryId
Empty match entryId LibrariesResource.runQuery emits entryId as entry.getCitationKey().orElse(""), so entries without a citation key become indistinguishable (""), breaking the endpoint’s stated identification-by-key contract.

Issue description

POST /libraries/query returns LibraryQueryMatch.entryId as an empty string when an entry has no citation key. This makes results ambiguous (collisions) and violates the DTO’s documented intent (identify by citation key).

Issue Context

  • LibraryQueryMatch is documented as identifying a match by "library and citation key".
  • Citation keys are optional in the model; therefore, returning "" is a real runtime state.

Fix Focus Areas

  • jabsrv/src/main/java/org/jabref/http/server/resources/LibrariesResource.java[88-94]
  • jabsrv/src/main/java/org/jabref/http/dto/LibraryQueryMatch.java[5-8]

Suggested fix

Choose one:

  1. Change LibraryQueryMatch to carry a stable internal identifier (e.g., BibEntry.getId()) and optionally add citationKey as a separate field.
  2. If the API must be citation-key based, filter out entries without citation keys (or generate keys before responding) and document that behavior.
                     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.

Issue description

The newly added changelog entry is missing the required issue/PR reference, reducing traceability and making the changelog inconsistent.

Issue Context

In CHANGELOG.md, entries should reference an issue (#NUM) or link the PR implementing the change.

Fix Focus Areas

  • 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).

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[286-296]

Suggested fix

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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[339-362]
  • jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[502-520]

Suggested fix options

Pick one approach:

  1. 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.
  2. 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.
  3. If keeping a lock, replace it with an explicit non-reentrant “dialog in progress” gate + queue; synchronized alone 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.

Issue description

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.

Issue Context

  • InternalField.KEY_FIELD is the citation key; StandardField.KEY is 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.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/model/entry/BibEntry.java[188-198]

Suggested fix

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.

Issue description

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.

Issue Context

This change is in .github/PULL_REQUEST_TEMPLATE.md under the ### PR Description guidance.

Fix Focus Areas

  • .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.

Issue description

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.

Issue Context

  • 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.properties path earlier, but the newly added “Removing an obsolete key” step uses src/main/resources/..., creating an internal inconsistency.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

The actual test class is in package org.jabref.logic.l10n.

Fix Focus Areas

  • 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.

Issue description

LibraryTab#setDatabaseContext uses tabPane.getSelectionModel().selectedItemProperty().get().equals(this) which will throw if the selected item is null.

Issue Context

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.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/LibraryTab.java[372-382]

Suggested fix

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.

Issue description

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.

Issue Context

The parent check command explicitly documents 1 = findings and combines exit codes using Math.max(...), but CheckIntegrity.execute(...) never returns 1.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/commands/CheckIntegrity.java[83-112]
  • jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[50-54]

Implementation notes

  • After writing findings, return messages.isEmpty() ? 0 : 1.
  • Update the execute Javadoc to include 1 = findings for consistency with Check and CheckConsistency.
[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.

Issue description

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.

Issue Context

CitationKeyDuplicationChecker emits IntegrityMessage(..., StandardField.KEY) for duplicate citation keys.

Fix Focus Areas

  • 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]

Implementation notes

  • Treat StandardField.KEY the same as InternalField.KEY_FIELD for formatting (do not append :field).
  • Optionally, for key-level findings use parserResult.getCompleteEntryIndicator(entry) for the range, to avoid depending on field-alias resolution for StandardField.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.

Issue description

A user-facing CLI message is printed as a hardcoded English string, bypassing JabRef localization.

Issue Context

Other jabkit commands print user-visible output via Localization.lang(...). The new check command should follow the same convention.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

jbang-pr still performs checkout and uses setup-gradle (which installs JDK and JBang), but jbang-main no longer does.

Fix Focus Areas

  • .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.

Issue description

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.

Issue Context

Several other commands updated in this PR already use System.err.println(...) for the same error paths.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/commands/PdfUpdate.java[69-83]
  • jabkit/src/main/java/org/jabref/toolkit/commands/Convert.java[59-68]

Implementation notes

  • Replace the two System.out.println(...) calls in the error paths with System.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.

Issue description

New/modified code uses Optional.isEmpty() and then Optional.get(), violating the Optional-control-flow compliance rule.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

The help/description string for the picocli option --importToOpen claims it is the same as --importBibtex, but the code paths take different input types:

  • --importToOpen expects a file path or URL to import.
  • --importBibtex expects a BibTeX string. This makes the CLI usage message incorrect and can cause wrong usage.

Issue Context

ArgumentProcessor routes these two options to different UiCommands, and the Windows native host script demonstrates that --importToOpen is invoked with a tempfile path.

Fix Focus Areas

  • 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).

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

The newly added CHANGELOG bullet is grammatically incorrect/unpolished and should be rewritten in professional, end-user-friendly language.

Issue Context

Compliance requires professional and consistent user-facing documentation.

Fix Focus Areas

  • 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.

Issue description

A newly added comment misspells Java terminology as hashcode.

Issue Context

JabRef code style requires correct spelling/terminology in modified code and comments.

Fix Focus Areas

  • 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()`.

Issue description

AGENTS.md currently advises using Optional.get() “if really present”, which is discouraged by the compliance rules favoring idiomatic Optional control flow.

Issue Context

This guidance is used to steer contributors/agents; recommending get() can lead to fragile code patterns and conflicts with the project’s Optional conventions.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

The same command is duplicated in CHECKLIST.md, so it should be updated there too.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

The project ADR explicitly frames JSpecify usage as progressive annotation of APIs for compile-time checking.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

  • BracketedPattern.getFieldValue routes auth\d+ and auth\d+_\d+ patterns to authNofMth, which performs the truncation.
  • CitationKeyGenerator.expandBracketContent subsequently calls removeUnwantedCharacters(...), which invokes StringUtil.replaceSpecialCharacters(...) using UnicodeToReadableCharMap (e.g., \u00F6 → oe).
  • Because there is no re-truncation after normalization, the final output can exceed the intended N characters and differ from prior behavior.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/citationkeypattern/BracketedPattern.java[960-978]

How to fix

  • Ensure the same normalization used for citation keys (at minimum StringUtil.replaceSpecialCharacters, and ideally the full unwanted/disallowed filtering logic) is applied before taking substring(0, n) in authNofMth.
  • 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.

Issue description

BracketedPattern.authNofMth embeds the unwanted-characters list directly in the regex literal ([-ʹ:!;?^$]*`). This duplicates the default unwanted-characters definition maintained elsewhere.

Issue Context

Duplicated business rules (like "which characters are unwanted") can easily diverge over time and make bug fixes incomplete.

Fix Focus Areas

  • 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.

Issue description

CitationKeyGenerator.cleanKey currently removes whitespace via String.replaceAll("\\s", ""), which recompiles the pattern per call.

Issue Context

The compliance checklist requires using compiled Pattern instances for repeated/non-trivial regex operations.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyPatternPreferences.java[59-74]
  • jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyGenerator.java[28-36]

Suggested fix

  • Move the default unwanted-characters constant into CitationKeyPatternPreferences (or a small dedicated CitationKeyDefaults/CitationKeyConstants class in the same package).
  • Update CitationKeyGenerator to 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.

Issue description

CHANGELOG.md has an added blank line near the end of the file.

Issue Context

The changelog rules require not adding extra blank lines.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

  • captureFocusedField() sets lastFocusedFieldName right before calling selectNextEntry() / selectPreviousEntry().
  • setCurrentlyEditedEntry() has an early-return when the entry instance is unchanged, which bypasses the clearing logic.
  • LibraryTab.selectNextEntry/selectPreviousEntry move by index +/- 1 without explicit bounds checks, so it is possible for selection not to move at table bounds.

Fix Focus Areas

  • 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]

Suggested fix

  • Ensure lastFocusedFieldName is cleared when setCurrentlyEditedEntry exits early due to Objects.equals(this.currentlyEditedEntry, currentlyEditedEntry).
  • Optionally, also set lastFocusedFieldName = null at the start of captureFocusedField() 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.

Issue description

New comments are trivial (what-not-why) and one uses a non-standard /// comment marker.

Issue Context

Project compliance requires following code style and avoiding comments that merely restate the code.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[322-327]
  • jabgui/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java[535-540]

Clone this wiki locally