diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 84aabdb..a149456 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,9 +9,21 @@ on: - cron: '0 4 * * 1' workflow_dispatch: +permissions: + contents: read + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: build: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} + timeout-minutes: 45 steps: - name: Checkout code @@ -27,20 +39,23 @@ jobs: - name: Setup Gradle uses: gradle/actions/setup-gradle@v4 - - name: Grant execute permission for gradlew - run: chmod +x gradlew - - - name: Build with Gradle + - name: Build with Gradle (Linux) + if: runner.os == 'Linux' run: ./gradlew build - - name: Verify with Spotless and SpotBugs - run: ./gradlew check --no-configuration-cache + - name: Build with Gradle (Windows) + if: runner.os == 'Windows' + run: .\gradlew.bat build + + - name: Check release helpers + if: runner.os == 'Linux' + run: ./release-lib-test.sh - name: Upload test results uses: actions/upload-artifact@v4 if: always() with: - name: test-results + name: test-results-${{ matrix.os }} path: '**/build/reports/tests/' retention-days: 7 @@ -48,12 +63,13 @@ jobs: uses: actions/upload-artifact@v4 if: always() with: - name: static-analysis-reports + name: static-analysis-reports-${{ matrix.os }} path: '**/build/reports/spotbugs/' retention-days: 7 dependency-check: runs-on: ubuntu-latest + timeout-minutes: 90 if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' steps: @@ -70,11 +86,26 @@ jobs: - name: Setup Gradle uses: gradle/actions/setup-gradle@v4 + - name: Scan dependencies for known vulnerabilities + env: + NVD_API_KEY: ${{ secrets.NVD_API_KEY }} + run: ./gradlew dependencyCheckAggregate --no-configuration-cache + - name: Check for dependency updates + if: always() run: ./gradlew dependencyUpdates + - name: Upload dependency-check report + uses: actions/upload-artifact@v4 + if: always() + with: + name: dependency-check-report + path: 'build/reports/dependency-check/' + retention-days: 30 + - name: Upload dependency report uses: actions/upload-artifact@v4 + if: always() with: name: dependency-updates-report path: 'build/dependencyUpdates/' diff --git a/.gitignore b/.gitignore index ce03c50..4cedd1e 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ build /.idea/ /.codex **/.DS_Store +/release.md.?????? diff --git a/AGENTS.md b/AGENTS.md index efb2ad3..850e406 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,10 +7,12 @@ - Gradle wrappers (`./gradlew`, per-module wrappers) and `gradle.properties` are the expected entry points; avoid invoking system Gradle directly. ## Build, Test, and Development Commands -- `./gradlew build` — compile all modules, run tests where present. +- `./gradlew build` — compile all modules, run tests, Spotless, and SpotBugs (both static checks are part of `check`). +- `./check.sh` — test the release helpers, reformat with Spotless, then run the full `check`; use this before pushing. - `./gradlew test` or `./gradlew :gi-fx:test` — run the full suite or a single module; tests execute on JUnit Platform. - `./gradlew :gi-swing:shadowJar` (likewise `:gi-fx:shadowJar`, `:gi-console:shadowJar`) — produce self-contained artifacts (`*-fatjar.jar`). Note these bundle the runtime classpath but not Groovy, which is a `compileOnly` dependency the consumer supplies. - `./gradlew dependencyUpdates` — check for newer dependencies with the versions plugin. +- `./checkCVE.sh` — run a fresh aggregate CVE scan after exporting `NVD_API_KEY`. - Use a JDK with JavaFX when building/running `gi-fx` (e.g., Bellsoft Full JDK); `gi-swing` and `gi-console` work on any Java 21 JDK. ## Coding Style & Naming Conventions diff --git a/README.md b/README.md index 017f1e1..405a3ef 100644 --- a/README.md +++ b/README.md @@ -85,14 +85,27 @@ println("File chosen was $file") ./gradlew build ``` -When `org.gradle.configuration-cache=true` is enabled, `build` skips applying Spotless and SpotBugs because their current Gradle tasks are not configuration-cache compatible on Gradle 9. CI therefore runs both commands, and so should you before pushing: +`build` runs tests, Spotless, and SpotBugs, including when Gradle's configuration +cache is enabled. Before pushing, you can run the formatting-and-check wrapper: ```bash -./gradlew build -./gradlew check --no-configuration-cache +./check.sh ``` -`check.sh` runs the second command with `spotlessApply` so formatting is fixed rather than merely reported. +`check.sh` tests the release helpers, then runs `spotlessApply` and `check`. + +`urlExists` reports unsupported URL schemes and broken redirect chains as +warnings. Connection and HTTP failures are logged at debug level. To see those +details, set the matrix logger to DEBUG and enable `FINE` for the underlying +Java Util Logging configuration. + +Table views use `Table` as the window or tab title when neither a nonblank +explicit title nor a matrix name is available. + +CI builds all four modules on Linux and Windows. The full dependency CVE scan +runs on the weekly schedule or by manual dispatch because its NVD database +refresh needs an API key and can be rate limited; it is not a PR gate. Run +`./checkCVE.sh` with `NVD_API_KEY` set for a fresh scan before release. ## Requirements diff --git a/TODO.md b/TODO.md index 1a65d74..42e3045 100644 --- a/TODO.md +++ b/TODO.md @@ -14,9 +14,20 @@ See `req/2026-09-25-fixes.md` for the plan and `release.md` for the changelog. - [x] Release: `release.sh` version read-back and partial-publish reporting - [x] Docs: sample scripts updated to 0.4.0 and current Matrix package names +## Review fixes (2026-09-26) +See `req/v0.4.1-fixes.md` for the plan and `release.md` for the changelog. +- [x] Apply Java 21 toolchains to every module +- [x] Include Spotless and SpotBugs in default `check` +- [x] Scan module dependencies with `dependencyCheckAggregate` +- [x] Resolve JavaFX natives per OS and architecture +- [x] Guard null file names, password EOF, and null matrices +- [x] Merge duplicate fat-JAR metadata +- [x] Promote Unreleased notes and validate release versions +- [x] Diagnose `urlExists` failures and cover both shell platforms +- [x] Define the intended null-handling behavior for console `display(String)` — a null name reports `File name is null` without throwing. + ## Deferred -- [ ] Define the intended null-handling behavior for console `display(String)`. - [ ] Tidy the cosmetic Swing dialog title inconsistency. - [ ] Finish the root `test.groovy` sample update. - [ ] Review whether JavaFX `TableData` should remain publicly visible. diff --git a/build.gradle b/build.gradle index c4c6795..15ed37e 100644 --- a/build.gradle +++ b/build.gradle @@ -10,29 +10,34 @@ plugins { id('io.github.gradle-nexus.publish-plugin') version '2.0.0' id "io.github.ben-manes.versions" version "0.64.0" id "org.owasp.dependencycheck" version "13.0.0" - id "com.diffplug.spotless" version "8.10.2" apply false - id "com.github.spotbugs" version "6.5.11" apply false + id "com.diffplug.spotless" version "8.10.3" + id "com.github.spotbugs" version "6.5.12" apply false } group = 'se.alipsa.gi' version = '0.4.1-SNAPSHOT' description = 'Allows Gade Gui Interactive capabilities from a standalone app' -def requestedTaskNames = gradle.startParameter.taskNames.collect { it.toLowerCase(Locale.ROOT) } -def configurationCacheRequested = gradle.startParameter.configurationCacheRequested -def enableSpotless = !configurationCacheRequested || requestedTaskNames.any { it.contains('spotless') } -def enableSpotbugs = !configurationCacheRequested || requestedTaskNames.any { it.contains('spotbugs') } - java { toolchain { languageVersion.set(JavaLanguageVersion.of(21)) } } +spotless { + groovyGradle { + target 'build.gradle' + leadingTabsToSpaces(2) + trimTrailingWhitespace() + endWithNewline() + } +} + subprojects { group = rootProject.group version = rootProject.version - if (it.plugins.hasPlugin('java')) { + // Configure after each module applies Java; this block runs before module scripts. + pluginManager.withPlugin('java') { java { toolchain { languageVersion.set(JavaLanguageVersion.of(21)) @@ -40,50 +45,46 @@ subprojects { } } - if (enableSpotless) { - apply plugin: 'com.diffplug.spotless' - spotless { - java { - googleJavaFormat('1.34.0') - } - groovy { - excludeJava() - leadingTabsToSpaces(2) - trimTrailingWhitespace() - endWithNewline() - } - groovyGradle { - leadingTabsToSpaces(2) - trimTrailingWhitespace() - endWithNewline() - } + apply plugin: 'com.diffplug.spotless' + spotless { + java { + googleJavaFormat('1.34.0') + } + groovy { + excludeJava() + leadingTabsToSpaces(2) + trimTrailingWhitespace() + endWithNewline() } - tasks.matching { it.name.startsWith('spotless') }.configureEach { - notCompatibleWithConfigurationCache("Spotless does not support configuration cache yet") + groovyGradle { + leadingTabsToSpaces(2) + trimTrailingWhitespace() + endWithNewline() } } - if (enableSpotbugs) { - apply plugin: 'com.github.spotbugs' - spotbugs { - toolVersion = '4.10.4' - effort = 'max' - reportLevel = 'medium' - excludeFilter = rootProject.file('config/spotbugs-exclude.xml') + apply plugin: 'com.github.spotbugs' + spotbugs { + toolVersion = '4.10.4' + effort = 'max' + reportLevel = 'medium' + excludeFilter = rootProject.file('config/spotbugs-exclude.xml') + } + tasks.withType(com.github.spotbugs.snom.SpotBugsTask).configureEach { + reports { + html.required = true + xml.required = false } + } + // The SpotBugs plugin assigns auxClassPaths during project evaluation. + // Register this after it has applied so the full compile and runtime paths win. + afterEvaluate { tasks.withType(com.github.spotbugs.snom.SpotBugsTask).configureEach { spotbugsTask -> - notCompatibleWithConfigurationCache("SpotBugs does not support configuration cache yet") - String suffix = spotbugsTask.name.startsWith('spotbugs') ? - spotbugsTask.name.substring('spotbugs'.length()) : '' - String sourceSetName = suffix.isEmpty() ? 'main' : - suffix.substring(0, 1).toLowerCase(Locale.ROOT) + suffix.substring(1) + String suffix = spotbugsTask.name.substring('spotbugs'.length()) + String sourceSetName = suffix.substring(0, 1).toLowerCase(Locale.ROOT) + suffix.substring(1) def sourceSet = project.sourceSets.findByName(sourceSetName) if (sourceSet != null) { - auxClassPaths.from(sourceSet.compileClasspath, sourceSet.runtimeClasspath) - } - reports { - html.required = true - xml.required = false + spotbugsTask.auxClassPaths.setFrom(sourceSet.compileClasspath, sourceSet.runtimeClasspath) } } } @@ -131,13 +132,14 @@ tasks.named("dependencyUpdates").configure { } // OWASP Dependency Check configuration -// Run with: ./gradlew dependencyCheckAnalyze --rerun-tasks --no-configuration-cache --console=plain +// Run with: ./gradlew dependencyCheckAggregate --rerun-tasks --no-configuration-cache --console=plain +// The root has no dependencies; the single-project Analyze task scans nothing. // Reports are generated in build/reports/dependency-check/ dependencyCheck { // Read the NVD API key from the environment; never commit it to the repository. nvd.apiKey = providers.environmentVariable('NVD_API_KEY').orNull // Scan all configurations including test dependencies - scanConfigurations = ['runtimeClasspath', 'testRuntimeClasspath'] + scanConfigurations = ['compileClasspath', 'runtimeClasspath', 'testRuntimeClasspath'] // Fail the build if a CVSS score of 7 or higher is found (HIGH/CRITICAL) failBuildOnCVSS = 7.0f // Output formats: HTML, JSON, CSV, XML, SARIF, JUNIT, ALL diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle index 17d076c..821a390 100644 --- a/buildSrc/build.gradle +++ b/buildSrc/build.gradle @@ -2,6 +2,12 @@ plugins { id 'groovy-gradle-plugin' } +java { + toolchain { + languageVersion = JavaLanguageVersion.of(21) + } +} + repositories { gradlePluginPortal() } diff --git a/buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle b/buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle index cee4846..77449fb 100644 --- a/buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle +++ b/buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle @@ -39,10 +39,33 @@ tasks.named('shadowJar', com.github.jengelman.gradle.plugins.shadow.tasks.Shadow mergeGroovyExtensionModules() // Exclude signature files that cause issues when repackaging signed JARs - exclude 'META-INF/*.RSA', 'META-INF/*.SF', 'META-INF/*.DSA' + exclude 'META-INF/*.RSA', 'META-INF/*.SF', 'META-INF/*.DSA', 'META-INF/INDEX.LIST' // INCLUDE writes non-merged duplicates once per source jar. Module // descriptors are the only ones that matter here: a fat JAR is not a module, // and duplicate descriptors make jar tooling warn. exclude 'module-info.class', 'META-INF/versions/*/module-info.class' + + // Preserve bundled licence and notice texts without duplicate archive paths. + transform(com.github.jengelman.gradle.plugins.shadow.transformers.MergeLicenseResourceTransformer) { + artifactLicense = rootProject.file('LICENSE') + artifactLicenseSpdxId = 'MIT' + caseSensitive = false + include 'META-INF/LICENSE*', 'LICENSE*' + } + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'META-INF/NOTICE' + } + // Some bundled artifacts use these distinct paths. Merge each at its own + // path so their attribution survives without duplicate ZIP entries. + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'META-INF/NOTICE.txt' + } + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'NOTICE' + } + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'META-INF/DEPENDENCIES' + } + exclude 'META-INF/buildinfo.xml' } diff --git a/buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle b/buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle new file mode 100644 index 0000000..53dd4e3 --- /dev/null +++ b/buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle @@ -0,0 +1,47 @@ +/** Resolve the OpenJFX native classifier for the JVM running Gradle. */ +import org.gradle.internal.os.OperatingSystem + +def os = OperatingSystem.current() +def launcherMetadata = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(21) +}.get().metadata +def arch = System.getProperty('os.arch', '').toLowerCase(Locale.ROOT) +if (!launcherMetadata.currentJvm) { + // The JavaFX tests run with the toolchain JVM, which can have another + // architecture than the Gradle daemon (for example under Rosetta). + def releaseFile = launcherMetadata.installationPath.file('release').asFile + if (releaseFile.isFile()) { + def match = releaseFile.text =~ /(?m)^OS_ARCH="?([^"\r\n]+)"?$/ + if (match.find()) arch = match.group(1).toLowerCase(Locale.ROOT) + } +} +def arm = arch in ['aarch64', 'arm64'] +def x64 = arch in ['x86_64', 'amd64'] + +String classifier +if (os.linux && (arm || x64)) { + classifier = arm ? 'linux-aarch64' : 'linux' +} else if (os.macOsX && (arm || x64)) { + classifier = arm ? 'mac-aarch64' : 'mac' +} else if (os.windows && x64) { + classifier = 'win' +} else { + // Keep non-FX tasks usable; only FX dependency resolution should fail. + classifier = 'unknown' +} + +if (classifier == 'unknown') { + def unsupportedMessage = "No OpenJFX natives for ${os.name} (os.arch=${arch}). " + + 'Supported: Linux x64/aarch64, macOS x64/aarch64, Windows x64. ' + + 'On Windows Arm64 use an x64 JDK under emulation; an Arm64 JVM cannot load x64 natives. ' + + 'gi-fx and gi-swing cannot resolve OpenJFX.' + configurations.configureEach { configuration -> + configuration.incoming.beforeResolve { + if (configuration.allDependencies.any { it.group == 'org.openjfx' }) { + throw new GradleException(unsupportedMessage) + } + } + } +} + +ext.javafxClassifier = classifier diff --git a/check.sh b/check.sh index 40c24bb..234ee94 100755 --- a/check.sh +++ b/check.sh @@ -1,4 +1,6 @@ #!/usr/bin/env bash set -euo pipefail -./gradlew spotlessApply check --no-configuration-cache --console=plain +cd -- "$(dirname -- "${BASH_SOURCE[0]}")" +./release-lib-test.sh +./gradlew spotlessApply check --console=plain diff --git a/checkCVE.sh b/checkCVE.sh new file mode 100755 index 0000000..1d139fb --- /dev/null +++ b/checkCVE.sh @@ -0,0 +1,9 @@ +#!/usr/bin/env bash +set -euo pipefail + +if [ -z "${NVD_API_KEY:-}" ]; then + echo 'NVD_API_KEY is required; request one at https://nvd.nist.gov/developers/request-an-api-key' >&2 + exit 1 +fi + +./gradlew dependencyCheckAggregate --rerun-tasks --no-configuration-cache --console=plain diff --git a/docs/API-Guide.md b/docs/API-Guide.md index 87df881..395fefa 100644 --- a/docs/API-Guide.md +++ b/docs/API-Guide.md @@ -295,7 +295,7 @@ if (io.urlExists("https://example.com/api/health", 5000)) { } ``` -`urlExists` accepts HTTP and HTTPS URLs, follows redirects within the supplied overall timeout budget, and returns `false` unless the final response is 2xx. A negative timeout throws `IllegalArgumentException`; `0` disables the timeout entirely, so the call may block indefinitely. +`urlExists` accepts HTTP and HTTPS URLs, follows redirects within the supplied overall timeout budget, and returns `false` unless the final response is 2xx. A negative timeout throws `IllegalArgumentException`; `0` disables the timeout entirely, so the call may block indefinitely. Debug diagnostics include the original and current URL without query strings or credentials, even when a malformed URL places credentials in the path. ### Content Type Detection @@ -365,6 +365,7 @@ if (file == null) { - SVG rendering via matrix-charts JavaFX integration (a lighter subset than a browser renderer) - Rich date pickers with calendar UI - Table views tolerate ragged data: a row shorter than the header list renders empty cells, and a column with no declared type is treated as `STRING` and left-aligned. +- `view((Matrix) null)` is logged and ignored; an unnamed matrix uses the tab title `Table`. ### gi-swing @@ -373,12 +374,14 @@ if (file == null) { - Standard Swing look and feel - A resource is rendered as SVG when its detected content type is `image/svg+xml`, or when its filename ends in `.svg` and detection is absent or inconclusive (`application/xml` or `text/xml`). - Table columns are right-aligned when the first non-null value in that column is numeric. +- `view((Matrix) null)` is logged and ignored. ### gi-console - Best for headless/CI environments -- `display(File)` opens the file with the system's default application when the platform supports the Desktop OPEN action; otherwise it prints a message. A missing file, a null file, and a failed open are all reported on stdout rather than thrown. +- `display(File)` opens the file with the system's default application when the platform supports the Desktop OPEN action; otherwise it prints a message. A missing file, a null file or file name, and a failed open are all reported on stdout rather than thrown. - `display(JComponent)` and `display(Svg)` print messages instead of showing UI -- Password input is masked when `System.console()` is available; otherwise stdin input is visible and a warning is logged +- Password input is masked when `System.console()` is available; otherwise stdin input is visible and a warning is logged. End of input cancels either path and returns `null`. - Tables displayed as text using Matrix.content() +- `view((Matrix) null)` prints a message instead of throwing. - Console input is decoded with the `stdin.encoding` charset when the JVM reports one (JDK 25+); otherwise the platform console's own charset is used when a console is attached (JDK 17+); otherwise the JVM default charset. `sysin` can be reassigned to redirect input in tests. diff --git a/docs/DEPENDENCIES.md b/docs/DEPENDENCIES.md index 304430c..a944c41 100644 --- a/docs/DEPENDENCIES.md +++ b/docs/DEPENDENCIES.md @@ -91,10 +91,15 @@ The project includes the OWASP Dependency Check plugin to scan for known vulnera # https://nvd.nist.gov/developers/request-an-api-key) export NVD_API_KEY='your-nvd-api-key' -# Run the security scan -./gradlew dependencyCheckAnalyze --no-configuration-cache --console=plain +# Run the security scan across the root project and all four modules +./checkCVE.sh ``` +Use `dependencyCheckAggregate`, not `dependencyCheckAnalyze`: the root project +declares no dependencies, so the single-project task scans nothing. +The script checks for `NVD_API_KEY` and forces a fresh scan; for a direct invocation, +run `./gradlew dependencyCheckAggregate --no-configuration-cache --console=plain`. + Reports are written below `build/reports/dependency-check/`. The build fails when a dependency has a CVSS score of 7 or higher. @@ -132,9 +137,14 @@ Check for available updates: | GuiInteraction | Java | Groovy | JavaFX | |----------------|------|--------|--------| +| 0.4.x | 21+ | 5.1.x | 23 | +| 0.3.x | 21+ | 5.0.x | 21-23 | | 0.2.x | 21+ | 5.0.x | 21-23 | | 0.1.x | 17+ | 4.0.x | 17-21 | +Every module compiles with a Java 21 toolchain configured in the root build, +so published artifacts load on Java 21 and later runtimes. + ## CI/CD Integration The GitHub Actions workflow includes a separate `dependency-check` job that runs on a schedule or manual workflow dispatch: @@ -145,11 +155,18 @@ dependency-check: if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' steps: + - name: Scan dependencies for known vulnerabilities + env: + NVD_API_KEY: ${{ secrets.NVD_API_KEY }} + run: ./gradlew dependencyCheckAggregate --no-configuration-cache + - name: Check for dependency updates + if: always() run: ./gradlew dependencyUpdates ``` -This approach keeps regular CI builds fast while still providing periodic dependency monitoring. +This keeps regular CI fast while scanning for CVEs and dependency updates on a +schedule. The scan needs the `NVD_API_KEY` repository secret to avoid NVD rate limits. ## Module dependency scopes @@ -168,3 +185,18 @@ is `--fatjar.jar`. `mergeServiceFiles()` is required: several transitive `ph-*` (ph-css/gsvg) jars and `tika-core` declare the same `META-INF/services/*` paths, and a plain `Jar` task with `DuplicatesStrategy.EXCLUDE` keeps only the first and silently discards the rest. + +Shadow merges bundled licence, notice and dependency metadata into single entries +and excludes source-JAR `buildinfo.xml`; no duplicate archive paths remain. +Check with `unzip -l | awk 'NF>=4{print $4}' | sort | uniq -d`. + +## JavaFX platform natives + +The `se.alipsa.gi.javafx-platform` convention plugin selects OpenJFX natives from +the Gradle JVM's OS and architecture: `linux`, `linux-aarch64`, `mac`, +`mac-aarch64`, or `win`. On unsupported architectures, non-JavaFX tasks remain +available; resolving JavaFX dependencies fails with the `unknown` classifier. +Windows Arm64 needs an x64 JDK under emulation: an Arm64 JVM cannot load the +published x64 Windows JavaFX binaries. Keep the Gradle JVM and the selected +Java 21 toolchain on the same architecture; the classifier follows the Gradle +JVM, while JavaFX native loading follows the toolchain running the application. diff --git a/docs/PUBLISHING.md b/docs/PUBLISHING.md index ca6933d..50a82a2 100644 --- a/docs/PUBLISHING.md +++ b/docs/PUBLISHING.md @@ -129,6 +129,7 @@ export ORG_GRADLE_PROJECT_signingSecretKeyRingFile=/path/to/secring.gpg ### Using the Release Script The easiest way to publish is using the release script: +It selects the newest installed JavaFX-capable JDK 21 from SDKMAN. ```bash # Dry run (shows what would happen) @@ -274,3 +275,31 @@ original version. This re-runs the version/tag/test steps but publishes only the remaining modules, then continues on to pushing the commit and creating the GitHub release. + +## Release notes and version shape + +Write curated changes under `## Unreleased` in `release.md`. At release time, +`release.sh` promotes that heading to `## - ` and opens a fresh +empty `## Unreleased` section above it. When no Unreleased heading exists, the +script generates notes from commit subjects. Both paths are idempotent for a +version already in the changelog, so `--skip` retries do not duplicate notes. +If a retry finds additional curated notes under `## Unreleased`, the script +warns that they are not included in the already-recorded version. + +Commit curated notes before running `release.sh`: every release path requires +`release.md` to be clean. SNAPSHOT and `--bump` releases also require clean +`build.gradle` and `README.md`; an unchanged-version release requires a clean +`README.md` only if you accept its prompted version update. A dry run previews +release-file changes and warns about dirty files without modifying them. + +Run `./release-lib-test.sh` to check empty-Unreleased fallback, atomic +replacement and permission preservation, awk-failure handling, and release +commits that leave declined README edits untouched. The sourceable helpers in +`release-lib.sh` use return codes 0 for promotion, 1 for no curated content, +2 for a version already recorded, and 3 for an I/O error. +An empty `## Unreleased` heading uses the commit-log fallback, which inserts the +new section beneath that heading. + +`release.sh` requires `build.gradle` to contain a +`MAJOR.MINOR.PATCH[-SNAPSHOT]` version with no leading zeros, and validates a +computed `--bump` version before changing release files. diff --git a/gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy b/gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy index 6c6aac7..52081b6 100644 --- a/gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy +++ b/gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy @@ -7,6 +7,7 @@ import org.commonmark.ext.gfm.tables.TablesExtension; import org.commonmark.parser.Parser; import org.commonmark.renderer.html.HtmlRenderer; import org.apache.tika.Tika +import se.alipsa.matrix.core.util.Logger import java.awt.Toolkit import java.awt.datatransfer.DataFlavor @@ -24,6 +25,7 @@ import java.util.concurrent.atomic.AtomicInteger @CompileStatic abstract class AbstractInOut implements GuiInteraction { + private static final Logger log = Logger.getLogger(AbstractInOut.class) private static final int MAX_REDIRECTS = 5 private Parser markdownParser @@ -40,12 +42,20 @@ abstract class AbstractInOut implements GuiInteraction { if (timeout < 0) { throw new IllegalArgumentException("timeout cannot be negative") } + URL originalUrl = null + URL url = null try { - URL url = new URL(urlString) + url = new URL(urlString) + originalUrl = url long deadline = timeout > 0 ? System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeout) : Long.MAX_VALUE for (int redirect = 0; redirect <= MAX_REDIRECTS; redirect++) { - if (!isHttpUrl(url) || !hasTimeRemaining(timeout, deadline)) { + if (!isHttpUrl(url)) { + logUrlExistsFailure(originalUrl, url, 'non-HTTP(S) URL') + return false + } + if (!hasTimeRemaining(timeout, deadline)) { + logUrlExistsFailure(originalUrl, url, 'timeout exhausted') return false } HttpURLConnection con = null @@ -54,6 +64,7 @@ abstract class AbstractInOut implements GuiInteraction { int responseCode = con.getResponseCode() if (shouldFallbackToGet(responseCode)) { if (!hasTimeRemaining(timeout, deadline)) { + logUrlExistsFailure(originalUrl, url, 'timeout exhausted before GET fallback') return false } con.disconnect() @@ -61,6 +72,7 @@ abstract class AbstractInOut implements GuiInteraction { responseCode = con.getResponseCode() if (responseCode == 416) { if (!hasTimeRemaining(timeout, deadline)) { + logUrlExistsFailure(originalUrl, url, 'timeout exhausted before range retry') return false } con.disconnect() @@ -71,24 +83,81 @@ abstract class AbstractInOut implements GuiInteraction { if (responseCode >= 300 && responseCode < 400) { String location = con.getHeaderField("Location") if (location == null || redirect == MAX_REDIRECTS) { + logUrlExistsFailure(originalUrl, url, + location == null ? 'redirect missing Location' : 'redirect limit exhausted') return false } url = new URL(url, location) continue } - return responseCode >= 200 && responseCode < 300 + boolean reachable = responseCode >= 200 && responseCode < 300 + if (!reachable) { + logUrlExistsFailure(originalUrl, url, 'got HTTP ' + responseCode) + } + return reachable } finally { if (con != null) { con.disconnect() } } } - } catch (RuntimeException | IOException ignored) { + } catch (RuntimeException | IOException e) { + String reason = 'failed: ' + e.getClass().getSimpleName() + exceptionDetail(e) + if (originalUrl == null) { + if (log.isDebugEnabled()) log.debug('urlExists(' + safeInputUrl(urlString) + ') ' + reason) + } else { + logUrlExistsFailure(originalUrl, url, reason) + } return false } + // Every reachable loop path returns; this satisfies the method's return type. return false } + @PackageScope + static String safeInputUrl(String input) { + if (input == null) return '' + String withoutQuery = input.split('[?#]', 2)[0] + String withoutCredentials = redactUserInfo(withoutQuery) + String clipped = withoutCredentials.length() <= 120 ? withoutCredentials : withoutCredentials.substring(0, 120) + '…' + return safeLogText(clipped) + } + + private static void logUrlExistsFailure(URL original, URL current, String reason) { + String message = urlFailureMessage(original, current, reason) + if (reason == 'non-HTTP(S) URL' || reason == 'redirect missing Location' || reason == 'redirect limit exhausted') { + log.warn(message) + } else if (log.isDebugEnabled()) log.debug(message) + } + + @PackageScope + static String urlFailureMessage(URL original, URL current, String reason) { + return 'urlExists(' + safeUrl(original) + ' -> ' + safeUrl(current) + ') ' + safeLogText(reason) + } + + private static String exceptionDetail(Exception e) { + String detail = e.message + if (detail == null || detail.isBlank()) return '' + // Exception messages can contain full URLs, including credentials or query strings. + String withoutUrls = detail.replaceAll('(?i)[a-z][a-z0-9+.-]*:/+[^\\s]+', '') + return ': ' + safeLogText(redactUserInfo(withoutUrls)) + } + + private static String redactUserInfo(String value) { + // Malformed URLs can put user info after one slash or in URL.path. + return value.replaceAll('(^|:/+|/)[^/]*@', '$1') + } + + private static String safeLogText(String value) { + return value.replace('%n', '% n').replaceAll('[\\r\\n\\t\\p{Cntrl}]', ' ') + } + + private static String safeUrl(URL url) { + if (url == null) return '' + String withoutQuery = url.protocol + '://' + url.host + (url.port < 0 ? '' : ':' + url.port) + url.path + return safeLogText(redactUserInfo(withoutQuery)) + } + private static HttpURLConnection open(URL url, String method, int timeout, boolean range) { HttpURLConnection con = (HttpURLConnection) url.openConnection() con.setInstanceFollowRedirects(false) @@ -280,12 +349,17 @@ abstract class AbstractInOut implements GuiInteraction { } private static List shellCommand(String command) { - if (isWindows()) { - String commandInterpreter = System.getenv('ComSpec') - commandInterpreter = commandInterpreter ?: 'cmd.exe' - return [commandInterpreter, '/d', '/s', '/c', command] - } - return ['/bin/sh', '-c', command] + return shellCommand(command, isWindows()) + } + + @PackageScope + static List shellCommand(String command, boolean windows) { + return shellCommand(command, windows, System.getenv('ComSpec')) + } + + @PackageScope + static List shellCommand(String command, boolean windows, String commandInterpreter) { + return windows ? [commandInterpreter?.trim() ?: 'cmd.exe', '/d', '/s', '/c', command] : ['/bin/sh', '-c', command] } @PackageScope diff --git a/gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy b/gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy index 461f1eb..4d93904 100644 --- a/gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy +++ b/gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy @@ -3,6 +3,9 @@ package se.alipsa.gi import groovy.transform.CompileStatic import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test +import org.junit.jupiter.api.condition.DisabledOnOs +import org.junit.jupiter.api.condition.EnabledOnOs +import org.junit.jupiter.api.condition.OS import org.junit.jupiter.api.io.TempDir import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.Arguments @@ -25,11 +28,62 @@ import java.util.concurrent.atomic.AtomicReference import java.util.concurrent.TimeoutException import java.util.stream.Stream -import static org.junit.jupiter.api.Assumptions.assumeFalse import static org.junit.jupiter.api.Assertions.* class AbstractInOutTest { + @Test + void malformedUrlDiagnosticsKeepTheTargetButNotCredentialsOrQuery() { + assertEquals('htp://example.com/path', AbstractInOut.safeInputUrl('htp://user:secret@example.com/path?token=secret#fragment')) + assertEquals('htp:/host/path', AbstractInOut.safeInputUrl('htp:/user:secret@host/path?token=secret')) + assertEquals('https:/host/path', AbstractInOut.safeInputUrl('https:/user:secret@host/path?token=secret')) + assertEquals('example.com/path', AbstractInOut.safeInputUrl('user:secret@example.com/path?token=abc')) + assertEquals('', AbstractInOut.safeInputUrl(null)) + assertEquals('http://example.com/a% nFINE: fake', + AbstractInOut.safeInputUrl('http://example.com/a%nFINE: fake')) + } + + @Test + void theWindowsShellUsesComSpecAndFallsBackWhenItIsMissing() { + assertEquals(['C:\\Windows\\System32\\cmd.exe', '/d', '/s', '/c', 'dir'], + AbstractInOut.shellCommand('dir', true, 'C:\\Windows\\System32\\cmd.exe')) + assertEquals(['cmd.exe', '/d', '/s', '/c', 'dir'], AbstractInOut.shellCommand('dir', true, null)) + assertEquals(['cmd.exe', '/d', '/s', '/c', 'dir'], AbstractInOut.shellCommand('dir', true, '')) + assertEquals(['cmd.exe', '/d', '/s', '/c', 'dir'], AbstractInOut.shellCommand('dir', true, ' ')) + } + + @Test + void theUnixShellRunsThroughBinSh() { + assertEquals(['/bin/sh', '-c', 'ls -a'], AbstractInOut.shellCommand('ls -a', false)) + assertEquals(['/bin/sh', '-c', 'ls -a'], AbstractInOut.shellCommand('ls -a', false, 'ignored')) + } + + @Test + @EnabledOnOs(OS.WINDOWS) + void theWindowsShellRunsThroughCmd() { + ShellResult result = inOut.shell('echo windows-shell') + assertTrue(result.stdout.contains('windows-shell')) + } + + @Test + void urlExistsStillReportsFalseWhenTheConnectionFails() { + assertFalse(inOut.urlExists('http://127.0.0.1:1/health', 500)) + } + + @Test + void urlFailureDiagnosticKeepsRedirectChainButScrubsSecretsAndControlSequences() { + String message = AbstractInOut.urlFailureMessage( + new URL('http://user:secret@example.com/start?token=private'), + new URL('http://example.com/a%nFINE:fake?token=private'), + 'redirect missing Location') + assertEquals('urlExists(http://example.com/start -> http://example.com/a% nFINE:fake) redirect missing Location', message) + assertEquals('urlExists(https:///host/start -> https:///other/end) redirect missing Location', + AbstractInOut.urlFailureMessage( + new URL('https:/user:secret@host/start?token=private'), + new URL('https:/other:password@other/end?token=private'), + 'redirect missing Location')) + } + @TempDir File tempDir @@ -61,6 +115,7 @@ class AbstractInOutTest { } @Test + @DisabledOnOs(OS.WINDOWS) void shPrintsStandardOutputToSystemOut() { PrintStream originalOut = System.out ByteArrayOutputStream outBytes = new ByteArrayOutputStream() @@ -77,6 +132,7 @@ class AbstractInOutTest { } @Test + @DisabledOnOs(OS.WINDOWS) void shQuietOverloadSuppressesLiveOutput() { PrintStream originalOut = System.out PrintStream originalErr = System.err @@ -106,6 +162,7 @@ class AbstractInOutTest { } @Test + @DisabledOnOs(OS.WINDOWS) void shellSuccessReportsExitCodeAndResultStringIsDiagnostic() { ShellResult result = inOut.shell('printf hello') @@ -126,16 +183,16 @@ class AbstractInOutTest { } @Test + @DisabledOnOs(OS.WINDOWS) void shellTimeoutStopsACommand() { - assumeFalse(AbstractInOut.isWindows()) assertThrows(TimeoutException.class) { inOut.shell('sleep 3', true, 100) } } @Test + @DisabledOnOs(OS.WINDOWS) void shellSupportsGlobsPipesAndRedirects(@TempDir File commandDir) { - assumeFalse(AbstractInOut.isWindows()) new File(commandDir, 'matrix-one.txt').text = 'one' new File(commandDir, 'other.txt').text = 'two' File outputFile = new File(commandDir, 'out.txt') @@ -150,8 +207,8 @@ class AbstractInOutTest { } @Test + @DisabledOnOs(OS.WINDOWS) void shStreamsOutputBeforeTheCommandCompletes() { - assumeFalse(AbstractInOut.isWindows()) PrintStream originalOut = System.out ByteArrayOutputStream bytes = new ByteArrayOutputStream() PrintStream capturedOut = new PrintStream(bytes, true, StandardCharsets.UTF_8) diff --git a/gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy b/gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy index 6b9cd62..1804718 100644 --- a/gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy +++ b/gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy @@ -7,6 +7,12 @@ import static org.junit.jupiter.api.Assertions.* class FileUtilsTest { + @Test + void aNullOrEmptyResourceNameResolvesToNullInsteadOfThrowing() { + assertNull(FileUtils.getResourceUrl(null)) + assertNull(FileUtils.getResourceUrl('')) + } + @TempDir File tempDir diff --git a/gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy b/gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy index 21aa228..a86a53a 100644 --- a/gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy +++ b/gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy @@ -194,13 +194,23 @@ class InOut extends AbstractInOut { } } + /** End of console input is cancellation, just as it is for the other prompts. */ + @PackageScope + static String toPassword(char[] password) { + if (password == null) return null + try { + return new String(password) + } finally { + Arrays.fill(password, (char) 0) + } + } + @Override String promptPassword(String title, String message) { println title def console = System.console() if (console != null) { - char[] ch = console.readPassword("$message : ") - return new String(ch) + return toPassword(console.readPassword("$message : ")) } // Fallback for IDEs/CI where System.console() is unavailable // Warning: input will be visible (not masked) @@ -260,6 +270,10 @@ class InOut extends AbstractInOut { @Override void view(Matrix tableMatrix, String... title) { + if (tableMatrix == null) { + println('Nothing to view: the matrix is null') + return + } println tableMatrix.content() } @@ -300,6 +314,10 @@ class InOut extends AbstractInOut { @Override void display(String fileName, String... title) { + if (fileName == null) { + println('File name is null') + return + } display(new File(fileName), title) } diff --git a/gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy b/gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy index ef49ed7..fcbc0c7 100644 --- a/gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy +++ b/gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy @@ -1,6 +1,7 @@ package se.alipsa.gi.console import org.junit.jupiter.api.Test +import org.junit.jupiter.api.Assumptions import java.nio.charset.Charset import java.nio.charset.StandardCharsets @@ -15,6 +16,33 @@ import static org.junit.jupiter.api.Assertions.assertTrue class ConsolePromptTest { + @Test + void anEofPasswordIsCancellationRatherThanACrash() { + assertNull(InOut.toPassword(null)) + char[] password = 's3cret'.toCharArray() + assertEquals('s3cret', InOut.toPassword(password)) + assertTrue(password.every { it == (char) 0 }) + } + + @Test + void theVisiblePasswordFallbackTreatsEofAsCancellation() { + Assumptions.assumeTrue(System.console() == null) + InOut inOut = new InOut() + inOut.sysin = new BufferedReader(new StringReader('')) + assertNull(inOut.promptPassword('title', 'password')) + } + + @Test + void viewingANullMatrixReportsItWithoutThrowing() { + assertTrue(captureStdout { new InOut().view((se.alipsa.matrix.core.Matrix) null) }.contains('null')) + } + + @Test + void displayingANullFileNameReportsItWithoutThrowing() { + assertEquals('File name is null', + captureStdout { new InOut().display((String) null) }.trim()) + } + @Test void eofIsCancellationForPrompts() { InOut inOut = new InOut() diff --git a/gi-fx/build.gradle b/gi-fx/build.gradle index 77bfbe5..bd46f8a 100644 --- a/gi-fx/build.gradle +++ b/gi-fx/build.gradle @@ -5,6 +5,7 @@ plugins { id 'maven-publish' id("se.alipsa.nexus-release-plugin") version '2.2.0' id 'se.alipsa.gi.fatjar-conventions' + id 'se.alipsa.gi.javafx-platform' } description = 'Allows Gade Gui Interactive capabilities from a javafx standalone app' @@ -29,17 +30,7 @@ java { } dependencies { - org.gradle.internal.os.OperatingSystem os = org.gradle.internal.os.OperatingSystem.current() - - // check https://repo1.maven.org/maven2/org/openjfx/javafx/20/javafx-20.pom for the platform property - def qualifier = 'unknown' - if (os.isLinux()) { - qualifier='linux' - } else if (os.isWindows()) { - qualifier = 'win' - } else if (os.isMacOsX()) { - qualifier = 'mac-aarch64' - } + def qualifier = javafxClassifier def javaFxVersion = libs.versions.javafx.get() api project(':gi-common') diff --git a/gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy b/gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy index 88ea04e..7c29abe 100644 --- a/gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy +++ b/gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy @@ -442,18 +442,11 @@ class InOut extends AbstractInOut { @Override void view(List> matrix, String... title) { - List> rows = rowsForViewer(matrix) Platform.runLater { - Viewer.viewTable(rows, title) + Viewer.viewTable(matrix, title) } } - /** Preserves ragged and null rows for the JavaFX table renderer. */ - @PackageScope - static List> rowsForViewer(List> matrix) { - return matrix - } - @Override void view(Matrix matrix, String... title) { Platform.runLater { diff --git a/gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy b/gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy index d7ad982..e6f8cd1 100644 --- a/gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy +++ b/gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy @@ -56,7 +56,7 @@ class Viewer { private static final Logger log = Logger.getLogger(Viewer.class) static final KeyCodeCombination KEY_CODE_COPY = - System.getProperty("os.name").toLowerCase().contains("mac") ? + System.getProperty("os.name", "").toLowerCase(Locale.ROOT).contains("mac") ? new KeyCodeCombination(KeyCode.C, KeyCombination.META_ANY) : new KeyCodeCombination(KeyCode.C, KeyCombination.CONTROL_ANY) @@ -194,15 +194,38 @@ class Viewer { } static void viewTable(Matrix tableMatrix, String... title) { - String tit = title.length > 0 ? title[0] : tableMatrix.matrixName - viewTable(tableMatrix.columnNames(), tableMatrix.rowList(), tableMatrix.typeNames(), tit) + if (tableMatrix == null) { + log.warn('matrix is null, nothing to view') + return + } + viewTable(tableMatrix.columnNames(), tableMatrix.rowList(), tableMatrix.typeNames(), + matrixTitle(tableMatrix, title)) + } + + @PackageScope + static String matrixTitle(Matrix matrix, String... title) { + if (title.length > 0 && title[0] != null && !title[0].trim().isEmpty()) { + return title[0] + } + String name = matrix?.matrixName + return tableTitle(name) + } + + @PackageScope + static String tableTitle(String... title) { + return title.length == 0 || title[0] == null || title[0].trim().isEmpty() ? 'Table' : title[0] } static void viewTable(Grid grid, String... title) { + if (grid == null) { + log.warn('Cannot view table: the grid is null') + return + } viewTable(grid.getRowList(), title) } static void viewTable(List> rows, String... title) { + // Keep ragged and null rows intact; formatting handles them at display time. if (rows == null || rows.isEmpty()) { viewTable(Collections.emptyList(), Collections.emptyList(), Collections.emptyList(), title) return @@ -302,10 +325,7 @@ class Viewer { } tableView.setItems(data) Tab tab = new Tab() - String tabTitle = " (" + rowList.size() + " rows)" - if (title.length > 0) { - tabTitle = title[0] + tabTitle - } + String tabTitle = tableTitle(title) + " (" + rowList.size() + " rows)" tab.setText(tabTitle) ScrollPane scrollPane = new ScrollPane(tableView) scrollPane.setVisible(true) @@ -315,7 +335,7 @@ class Viewer { SingleSelectionModel selectionModel = viewPane.getSelectionModel() selectionModel.select(tab) Alert alert = new Alert(Alert.AlertType.INFORMATION) - alert.setTitle(title.length > 0 ? title[0] : tabTitle) + alert.setTitle(tableTitle(title)) alert.setHeaderText(null) alert.setContentText(null) alert.getDialogPane().setContent(viewPane) diff --git a/gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy b/gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy index c59ae37..41f97a8 100644 --- a/gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy +++ b/gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy @@ -6,7 +6,6 @@ import se.alipsa.groovy.svg.Svg import static org.junit.jupiter.api.Assertions.assertEquals import static org.junit.jupiter.api.Assertions.assertFalse import static org.junit.jupiter.api.Assertions.assertNull -import static org.junit.jupiter.api.Assertions.assertSame /** * Unit tests for the pure helpers of {@link InOut}. The InOut constructor throws @@ -38,11 +37,4 @@ class InOutHelperTest { assertEquals('chosen', InOut.svgTitle(new Svg(), 'chosen')) } - @Test - void viewingAListPreservesRaggedAndNullRowsForTheViewer() { - List> rows = [['first'], null, ['second', 'third']] - - assertSame(rows, InOut.rowsForViewer(rows)) - } - } diff --git a/gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy b/gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy index 852ed5d..531e20f 100644 --- a/gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy +++ b/gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy @@ -1,13 +1,43 @@ package se.alipsa.gi.fx import org.junit.jupiter.api.Test +import se.alipsa.matrix.core.Matrix +import se.alipsa.matrix.core.Grid import java.text.NumberFormat import static org.junit.jupiter.api.Assertions.assertEquals +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow class TableDataTest { + @Test + void unnamedAndExplicitMatrixTitles() { + Matrix unnamed = Matrix.builder().rows([[1, 2]]).build() + Matrix named = Matrix.builder().matrixName('from matrix').rows([[1, 2]]).build() + assertEquals('Table', Viewer.matrixTitle(unnamed)) + assertEquals('Table', Viewer.matrixTitle(null)) + assertEquals('chosen', Viewer.matrixTitle(named, 'chosen')) + assertEquals('from matrix', Viewer.matrixTitle(named, ' ')) + assertEquals('Table', Viewer.tableTitle((String) null)) + assertEquals('Table', Viewer.tableTitle(' ')) + assertEquals('chosen', Viewer.tableTitle('chosen')) + } + + @Test + void viewingANullMatrixIsIgnoredRatherThanThrowing() { + assertDoesNotThrow({ Viewer.viewTable((Matrix) null) } as org.junit.jupiter.api.function.Executable) + assertDoesNotThrow({ Viewer.viewTable((Grid) null) } as org.junit.jupiter.api.function.Executable) + } + + @Test + void aRaggedGridKeepsEveryColumnAndRendersNullRowsAsEmpty() { + List> rows = [['first'], null, ['second', 'third']] + assertEquals(2, Viewer.widestRow(rows)) + assertEquals([], Viewer.formatRow(rows[1], NumberFormat.getInstance())) + assertEquals('', TableData.cellAt(Viewer.formatRow(rows[0], NumberFormat.getInstance()), 1)) + } + @Test void theGridColumnCountCoversTheWidestNonEmptyRow() { assertEquals(0, Viewer.widestRow([[]])) diff --git a/gi-swing/build.gradle b/gi-swing/build.gradle index 8f281a5..d4541ef 100644 --- a/gi-swing/build.gradle +++ b/gi-swing/build.gradle @@ -6,6 +6,7 @@ plugins { id 'signing' id("se.alipsa.nexus-release-plugin") version '2.2.0' id 'se.alipsa.gi.fatjar-conventions' + id 'se.alipsa.gi.javafx-platform' } description = 'Allows Gade Gui Interactive capabilities from a standalone app' @@ -26,17 +27,7 @@ tasks.named('javadocJar', Jar) { } dependencies { - org.gradle.internal.os.OperatingSystem os = org.gradle.internal.os.OperatingSystem.current() - - // check https://repo1.maven.org/maven2/org/openjfx/javafx/20/javafx-20.pom for the platform property - def qualifier = 'unknown' - if (os.isLinux()) { - qualifier='linux' - } else if (os.isWindows()) { - qualifier = 'win' - } else if (os.isMacOsX()) { - qualifier = 'mac-aarch64' - } + def qualifier = javafxClassifier def javaFxVersion = libs.versions.javafx.get() api project(':gi-common') diff --git a/gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy b/gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy index 2fd47ee..1cb4808 100644 --- a/gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy +++ b/gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy @@ -322,6 +322,7 @@ class InOut extends AbstractInOut { @Override void view(Matrix tableMatrix, String... title) { + if (warnIfNullMatrix(tableMatrix)) return Vector rows = new Vector(tableMatrix.rowCount()) List> values = [] tableMatrix.each { r -> @@ -337,10 +338,25 @@ class InOut extends AbstractInOut { JTable jTable = new JTable(rows, tableMatrix.columnNames() as Vector) jTable.setAutoResizeMode(JTable.AUTO_RESIZE_ALL_COLUMNS) - def name = title.length > 0 ? title[0] : tableMatrix.matrixName + String name = tableTitle(tableMatrix.matrixName, title) viewTable(jTable, rightAlignments(values, tableMatrix.columnNames().size()), name) } + @PackageScope + static boolean warnIfNullMatrix(Matrix tableMatrix) { + if (tableMatrix == null) { + log.warn('Cannot view table: the matrix is null') + return true + } + return false + } + + @PackageScope + static String tableTitle(String matrixName, String... title) { + if (title.length > 0 && title[0] != null && !title[0].trim().isEmpty()) return title[0] + return matrixName == null || matrixName.trim().isEmpty() ? 'Table' : matrixName + } + private viewTable(JTable jTable, List rightAlign, String title) { DefaultTableCellRenderer rightRenderer = new DefaultTableCellRenderer() rightRenderer.setHorizontalAlignment(JLabel.RIGHT) @@ -360,7 +376,7 @@ class InOut extends AbstractInOut { @Override void view(List> matrix, String... title) { - def name = title.length > 0 ? title[0] : "" + String name = tableTitle(null, title) if (matrix == null || matrix.isEmpty()) { viewTable(new JTable(new Vector(), new Vector()), [], name) return diff --git a/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHeadlessTest.groovy b/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHeadlessTest.groovy index 0a9a920..d83ad6b 100644 --- a/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHeadlessTest.groovy +++ b/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHeadlessTest.groovy @@ -1,11 +1,20 @@ package se.alipsa.gi.swing import org.junit.jupiter.api.Test +import se.alipsa.matrix.core.Matrix +import static org.junit.jupiter.api.Assertions.assertFalse import static org.junit.jupiter.api.Assertions.assertThrows +import static org.junit.jupiter.api.Assertions.assertTrue class InOutHeadlessTest { + @Test + void aNullMatrixIsRejectedBeforeSwingIsTouched() { + assertTrue(InOut.warnIfNullMatrix(null)) + assertFalse(InOut.warnIfNullMatrix(Matrix.builder().rows([[1]]).build())) + } + @Test void headlessConstructionReportsTheExpectedException() { assertThrows(UnsupportedOperationException) { diff --git a/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHelperTest.groovy b/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHelperTest.groovy index 97ccec3..e8723e6 100644 --- a/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHelperTest.groovy +++ b/gi-swing/src/test/groovy/se/alipsa/gi/swing/InOutHelperTest.groovy @@ -1,7 +1,6 @@ package se.alipsa.gi.swing import org.junit.jupiter.api.Test -import org.junit.jupiter.api.io.TempDir import se.alipsa.groovy.svg.Svg import javax.swing.JEditorPane @@ -18,6 +17,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue */ class InOutHelperTest { + @Test + void anUnnamedTableGetsAReadableWindowTitle() { + assertEquals('Table', InOut.tableTitle(null)) + assertEquals('Table', InOut.tableTitle(' ', (String) null)) + assertEquals('from matrix', InOut.tableTitle('from matrix', ' ')) + assertEquals('chosen', InOut.tableTitle(null, 'chosen')) + } + @Test void aMissingInitialDirectoryBecomesNullRatherThanThrowing() { assertNull(InOut.toInitialDirectory(null)) @@ -94,10 +101,10 @@ class InOutHelperTest { } @Test - void loadingARealPageReportsSuccess(@TempDir File tempDir) { - File page = new File(tempDir, 'page.html') - page.text = '

ok

' - + void loadingARealPageReportsSuccess() { + // JEditorPane may still be reading HTML when this method returns. A fixture + // avoids racing JUnit's temporary-directory deletion on Windows. + File page = new File(getClass().getResource('/view-page.html').toURI()) assertTrue(InOut.loadPage(new JEditorPane(), page)) } diff --git a/gi-swing/src/test/resources/view-page.html b/gi-swing/src/test/resources/view-page.html new file mode 100644 index 0000000..47002a9 --- /dev/null +++ b/gi-swing/src/test/resources/view-page.html @@ -0,0 +1,2 @@ + +

ok

diff --git a/release-lib-test.sh b/release-lib-test.sh new file mode 100755 index 0000000..d369895 --- /dev/null +++ b/release-lib-test.sh @@ -0,0 +1,130 @@ +#!/usr/bin/env bash +set -euo pipefail + +source "$(dirname "$0")/release-lib.sh" +sorted_jdks=$(printf '%s\n' 21.0.9.fx-librca 21.0.11.fx-librca 21.0.12.fx-librca 21.0.12.1.fx-librca | sort_java_versions) +[ "$sorted_jdks" = $'21.0.12.1.fx-librca\n21.0.12.fx-librca\n21.0.11.fx-librca\n21.0.9.fx-librca' ] +fixture_dir=$(mktemp -d) +trap 'rm -rf "$fixture_dir"' EXIT +notes="$fixture_dir/release.md" +awk() { + if [ "${FAIL_AWK:-false}" = true ]; then + printf '# partial output\n' + return 42 + fi + command awk "$@" +} + +printf '# Notes\n\n## Unreleased\n\n## 0.4.1 - 2026-09-26\n- old\n' > "$notes" +chmod 664 "$notes" +if promote_unreleased_section 0.5.0 2026-10-01 "$notes"; then + echo 'Empty Unreleased was promoted' >&2 + exit 1 +fi +TMPDIR="$fixture_dir" insert_release_section 0.5.0 2026-10-01 'abc123 new change' "$notes" +awk '/^## Unreleased$/ { unreleased = NR } /^## 0.5.0 / { current = NR } /^## 0.4.1 / { old = NR } END { exit !(unreleased < current && current < old) }' "$notes" +[ "$(ls -ld "$notes" | cut -c1-10)" = '-rw-rw-r--' ] +grep -q -- '- new change' "$notes" + +printf '# Notes\n\n## Unreleased\n- curated\n' > "$notes" +chmod 664 "$notes" +promote_unreleased_section 0.5.0 2026-10-01 "$notes" +[ "$(ls -ld "$notes" | cut -c1-10)" = '-rw-rw-r--' ] +grep -q -- '- curated' "$notes" +if has_curated_unreleased_notes "$notes"; then + echo 'Empty Unreleased was reported as curated' >&2 + exit 1 +fi +printf '# Notes\n\n## Unreleased\n- later note\n\n## 0.5.0 - 2026-10-01\n- curated\n' > "$notes" +has_curated_unreleased_notes "$notes" +rc=0 +promote_unreleased_section 0.5.0 2026-10-01 "$notes" || rc=$? +[ "$rc" -eq 2 ] + +before=$(cat "$notes") +FAIL_AWK=true +if TMPDIR="$fixture_dir" insert_release_section 0.6.0 2026-10-02 'def456 next' "$notes"; then + echo 'Failed awk unexpectedly succeeded' >&2 + exit 1 +fi +FAIL_AWK=false +[ "$(cat "$notes")" = "$before" ] +printf '# Notes\n\n## Unreleased\n- curated\n' > "$notes" +before=$(cat "$notes") +mv() { return 42; } +rc=0 +promote_unreleased_section 0.6.0 2026-10-02 "$notes" || rc=$? +if [ "$rc" -ne 3 ]; then + echo "Failed rename returned $rc instead of 3 for promotion" >&2 + exit 1 +fi +[ "$(cat "$notes")" = "$before" ] +rc=0 +TMPDIR="$fixture_dir" insert_release_section 0.6.0 2026-10-02 'def456 next' "$notes" || rc=$? +if [ "$rc" -ne 3 ]; then + echo "Failed rename returned $rc instead of 3 for insertion" >&2 + exit 1 +fi +unset -f mv +[ "$(cat "$notes")" = "$before" ] +if find "$fixture_dir" -maxdepth 1 -name 'release.md.*' | grep -q .; then + echo 'Failed update left a temporary changelog behind' >&2 + exit 1 +fi + +printf '# Notes\r\n\r\n## Unreleased\r\n- curated\r\n' > "$notes" +promote_unreleased_section 0.7.0 2026-10-03 "$notes" +if perl -ne 'exit 1 if /(?&2 + exit 1 +fi +printf '# Notes\r\n\r\n## Unreleased\r\n' > "$notes" +TMPDIR="$fixture_dir" insert_release_section 0.7.0 2026-10-03 $'abc123 valid\nabc123\nabc123 ' "$notes" +grep -q -- '- valid' "$notes" +if grep -q -- '- abc123' "$notes"; then exit 1; fi +if perl -ne 'exit 1 if /(?&2 + exit 1 +fi +if find "$fixture_dir" -maxdepth 1 -type f ! -name release.md | grep -q .; then + echo 'Release helper left a temporary file behind' >&2 + exit 1 +fi + +repo="$fixture_dir/repo" +mkdir "$repo" +( + cd "$repo" + export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 + git init -q + git config user.name 'Release fixture' + git config user.email 'release-fixture@example.invalid' + git config commit.gpgsign false + git config core.hooksPath /dev/null + printf 'README baseline\n' > README.md + printf 'Release baseline\n' > release.md + printf 'Build baseline\n' > build.gradle + git add README.md release.md build.gradle + git commit -qm 'baseline' + require_clean_release_file release.md + printf 'Unrelated README work\n' >> README.md + if require_clean_release_file README.md 2>/dev/null; then + echo 'Dirty README passed preflight' >&2 + exit 1 + fi + printf 'Release notes\n' >> release.md + if require_clean_release_file release.md 2>/dev/null; then + echo 'Dirty release notes passed preflight' >&2 + exit 1 + fi + commit_changed_release_files 0.5.0 false >/dev/null + [ "$(git show --pretty=format: --name-only HEAD | sed '/^$/d')" = release.md ] + grep -q 'Unrelated README work' README.md + [ "$(git status --short)" = ' M README.md' ] + printf 'Unrelated build work\n' >> build.gradle + if require_clean_release_file build.gradle 2>/dev/null; then + echo 'Dirty build script passed preflight' >&2 + exit 1 + fi +) +echo 'release-lib fixtures passed' diff --git a/release-lib.sh b/release-lib.sh new file mode 100644 index 0000000..b4cc4c2 --- /dev/null +++ b/release-lib.sh @@ -0,0 +1,138 @@ +#!/usr/bin/env bash +# Sourceable changelog and version helpers for release.sh. + +# SDKMAN names start with numeric Java version components. Sort those components +# in descending order without relying on GNU sort -V (unavailable on macOS). +sort_java_versions() { + LC_ALL=C sort -t. -k1,1nr -k2,2nr -k3,3nr -k4,4nr +} + +release_section_exists() { + local version=$1 file=${2:-release.md} + [ -f "$file" ] && grep -qE "^## +${version//./[.]} - " "$file" +} + +has_curated_unreleased_notes() { + local file=${1:-release.md} + [ -f "$file" ] || return 1 + awk ' + /^## +Unreleased[[:space:]]*$/ { inside = 1; next } + inside && /^## / { exit } + inside && /[^[:space:]]/ { content = 1 } + END { exit content ? 0 : 1 } + ' "$file" +} + +# 0: promoted, 1: no curated Unreleased content, 2: version already recorded, 3: I/O error. +promote_unreleased_section() ( + local version=$1 date=$2 file=${3:-release.md} tmp + [ -f "$file" ] || return 1 + release_section_exists "$version" "$file" && return 2 + has_curated_unreleased_notes "$file" || return 1 + tmp=$(mktemp "${file}.XXXXXX") || return 3 + trap 'rm -f "$tmp"' EXIT + trap 'exit 130' INT TERM + local ending=$'\n' + grep -q $'\r' "$file" && ending=$'\r\n' + awk -v heading="## ${version} - ${date}" -v ending="$ending" ' + BEGIN { ORS = ending } + { sub(/\r$/, "") } + !promoted && /^## +Unreleased[[:space:]]*$/ { + print "## Unreleased"; print ""; print heading; promoted = 1; next + } + { print } + ' "$file" > "$tmp" || { rm -f "$tmp"; return 3; } + local mode + mode=$(stat -c %a "$file" 2>/dev/null || stat -f %Lp "$file") || { rm -f "$tmp"; return 3; } + chmod "$mode" "$tmp" || { rm -f "$tmp"; return 3; } + mv "$tmp" "$file" || { rm -f "$tmp"; return 3; } +) + +# Fallback when there are no curated notes. printf preserves backslashes in subjects. +insert_release_section() ( + local version=$1 date=$2 commits=$3 file=${4:-release.md} section tmp='' line + release_section_exists "$version" "$file" && return 2 + section=$(mktemp) || return 3 + trap 'rm -f "$section" "$tmp"' EXIT + trap 'exit 130' INT TERM + local ending=$'\n' + if [ -f "$file" ] && grep -q $'\r' "$file"; then ending=$'\r\n'; fi + { + printf '## %s - %s%s%s' "$version" "$date" "$ending" "$ending" + if [ -n "$commits" ]; then + printf '### Changes%s%s' "$ending" "$ending" + while IFS= read -r line; do + [[ "$line" == *' '* ]] || continue + local subject=${line#* } + [[ "$subject" =~ [^[:space:]] ]] && printf -- '- %s%s' "$subject" "$ending" + done <<< "$commits" + printf '%s' "$ending" + fi + } > "$section" || { rm -f "$section"; return 3; } + if [ ! -f "$file" ]; then + printf '# Gui Interaction Release Notes\n\n' > "$file" || { rm -f "$section"; return 3; } + fi + tmp=$(mktemp "${file}.XXXXXX") || { rm -f "$section"; return 3; } + awk -v section="$section" -v ending="$ending" ' + BEGIN { ORS = ending } + { sub(/\r$/, "") } + !inserted && /^## +Unreleased[[:space:]]*$/ { + print; next + } + !inserted && /^## / { + while ((getline line < section) > 0) { sub(/\r$/, "", line); print line } + close(section); inserted = 1 + } + { print } + END { + if (!inserted) { + while ((getline line < section) > 0) { sub(/\r$/, "", line); print line } + close(section) + } + } + ' "$file" > "$tmp" || { rm -f "$tmp" "$section"; return 3; } + local mode + mode=$(stat -c %a "$file" 2>/dev/null || stat -f %Lp "$file") || { rm -f "$tmp" "$section"; return 3; } + chmod "$mode" "$tmp" || { rm -f "$tmp" "$section"; return 3; } + mv "$tmp" "$file" || { rm -f "$tmp" "$section"; return 3; } + rm -f "$section" +) + +validate_version() { + local version=${1%-SNAPSHOT} + if [[ ! "$version" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then + echo "Unsupported version '${1}'. Expected MAJOR.MINOR.PATCH or MAJOR.MINOR.PATCH-SNAPSHOT, with no leading zeros." >&2 + return 1 + fi +} + +# Refuse to overwrite a file carrying edits from before this release run. +require_clean_release_file() { + local file=$1 + if ! git diff --quiet HEAD -- "$file"; then + echo "${file} has pre-existing changes; commit or move them aside before releasing." >&2 + return 1 + fi +} + +# Commit only files this run was allowed to change. A declined README update +# must never sweep unrelated README edits into the release commit. +commit_changed_release_files() { + local version=$1 include_readme=$2 + local -a files=() + if [ "$include_readme" = true ] && ! git diff --quiet HEAD -- README.md; then + files+=(README.md) + fi + if ! git diff --quiet HEAD -- release.md; then + files+=(release.md) + fi + [ "${#files[@]}" -gt 0 ] || return 0 + if ! git add "${files[@]}"; then + echo "Failed to stage release files for ${version}" >&2 + return 1 + fi + if ! git commit -m "Update release files for ${version}" -- "${files[@]}"; then + echo "Failed to commit release files for ${version}; resolve the git error before retrying" >&2 + return 1 + fi +} diff --git a/release.md b/release.md index 1ffd6b6..8d2e37f 100644 --- a/release.md +++ b/release.md @@ -1,6 +1,13 @@ # Gui Interaction Release Notes ## Unreleased +- Fixed (build): pin Java 21 toolchains in all modules; run Spotless and SpotBugs in default builds; scan all modules with `dependencyCheckAggregate`. +- Fixed (build): select JavaFX natives for the Gradle JVM's OS and architecture; unsupported combinations fail when JavaFX dependencies resolve, leaving unrelated tasks available. +- Fixed: console null file names and password EOF no longer throw; null matrix views are guarded and unnamed JavaFX tables use `Table` as their title. +- Fixed: JavaFX copy shortcut detection uses locale-independent casing, and the redundant `rowsForViewer` helper was removed. +- Fixed (packaging): merge duplicate licence, notice, and dependency metadata in fat JARs. +- Fixed (release): promote curated Unreleased notes, preserve backslashes in commit subjects, handle release retries, and validate version shapes before mutating files. +- Added: debug diagnostics for `urlExists` failures and Windows shell branch coverage with a CI smoke job. - Build: upgraded Groovy to 5.1.3, Matrix BOM to 2.6.0, CommonMark to 0.30.0, jsoup to 1.23.2, JUnit to 6.1.3, Gradle Versions to 0.64.0, Spotless to 8.10.2, and SpotBugs (plugin/engine) to 6.5.11/4.10.4. - Common: added `sh` and `shell` methods for executing platform shell commands. `sh` returns captured standard output and can stream output while the command runs; `shell` returns a `ShellResult` containing standard output, standard error, exit code, and success status. Both support optional timeouts. These methods execute arbitrary commands and should not receive untrusted input. - Breaking (packaging): fat JARs are now built with GradleUp Shadow via the `shadowJar` task instead of a hand-rolled `fatJar` task. The published artifact name is unchanged (`--fatjar.jar`), but the Gradle task is now `shadowJar`. diff --git a/release.sh b/release.sh index de56e4c..07bc8b0 100755 --- a/release.sh +++ b/release.sh @@ -22,6 +22,11 @@ # set -e +SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) +cd "$SCRIPT_DIR" +# shellcheck source=release-lib.sh +source "${SCRIPT_DIR}/release-lib.sh" + # Colors for output RED='\033[0;31m' GREEN='\033[0;32m' @@ -35,16 +40,21 @@ if [ -f ~/.sdkman/bin/sdkman-init.sh ]; then sdk_java_dir="${SDKMAN_CANDIDATES_DIR:-$HOME/.sdkman/candidates}/java" java21_fx_candidates=() if [ -d "$sdk_java_dir" ]; then - while IFS= read -r candidate; do - java21_fx_candidates+=("$candidate") - done < <( - find "$sdk_java_dir" -mindepth 1 -maxdepth 1 -type d -printf '%f\n' | - awk 'tolower($0) ~ /^21([.-]|$)/ && tolower($0) ~ /fx/' | - sort -V -r - ) + for candidate_path in "$sdk_java_dir"/*; do + [ -d "$candidate_path" ] || continue + candidate=${candidate_path##*/} + if [[ "${candidate,,}" =~ ^21([.-]|$) ]] && [[ "${candidate,,}" == *fx* ]]; then + java21_fx_candidates+=("$candidate") + fi + done fi if [ "${#java21_fx_candidates[@]}" -gt 0 ]; then + sorted_java21_fx_candidates=() + while IFS= read -r candidate; do + sorted_java21_fx_candidates+=("$candidate") + done < <(printf '%s\n' "${java21_fx_candidates[@]}" | sort_java_versions) + java21_fx_candidates=("${sorted_java21_fx_candidates[@]}") echo "Installed JavaFX-capable JDK 21 candidates:" printf ' %s\n' "${java21_fx_candidates[@]}" echo "Using Java ${java21_fx_candidates[0]}" @@ -162,16 +172,16 @@ bump_version() { case $type in major) - major=$((major + 1)) + major=$((10#$major + 1)) minor=0 patch=0 ;; minor) - minor=$((minor + 1)) + minor=$((10#$minor + 1)) patch=0 ;; patch) - patch=$((patch + 1)) + patch=$((10#$patch + 1)) ;; *) echo -e "${RED}Invalid bump type: $type. Use major, minor, or patch${NC}" @@ -209,7 +219,8 @@ update_readme_version() { # Check if README.md has the correct version check_readme_version() { local expected_version=$1 - local readme_versions=$(grep -oE '(gi-(swing|fx|console):)[0-9]+\.[0-9]+\.[0-9]+(-SNAPSHOT)?' README.md | head -1 | sed -E 's/gi-(swing|fx|console)://') + local readme_versions + readme_versions=$(grep -oE '(gi-(swing|fx|console):)[0-9]+\.[0-9]+\.[0-9]+(-SNAPSHOT)?' README.md | head -1 | sed -E 's/gi-(swing|fx|console)://') if [ "$readme_versions" != "$expected_version" ]; then echo -e "${RED}Warning: README.md contains version '${readme_versions}' but releasing '${expected_version}'${NC}" @@ -221,44 +232,41 @@ check_readme_version() { # Generate release notes entry generate_release_notes() { local version=$1 - local date=$(date +%Y-%m-%d) + local date + date=$(date +%Y-%m-%d) local release_notes_file="release.md" + local rc=0 if [ ! -f "$release_notes_file" ]; then - echo "# Gui Interaction Release Notes" > "$release_notes_file" - echo "" >> "$release_notes_file" + printf '# Gui Interaction Release Notes\n\n' > "$release_notes_file" fi + promote_unreleased_section "$version" "$date" "$release_notes_file" || rc=$? + case $rc in + 0) echo -e "${GREEN}Promoted Unreleased notes for ${version}${NC}"; return ;; + 2) + echo -e "${YELLOW}Notes for ${version} already exist${NC}" + if has_curated_unreleased_notes "$release_notes_file"; then + echo -e "${YELLOW}Unreleased still has curated notes; they were not included in ${version}. Review before continuing.${NC}" >&2 + fi + return + ;; + 3) echo -e "${RED}Failed to update ${release_notes_file}${NC}" >&2; return 1 ;; + esac - # Get commits since last tag - local last_tag=$(git describe --tags --abbrev=0 2>/dev/null || echo "") - local commits="" + local last_tag commits + last_tag=$(git describe --tags --abbrev=0 2>/dev/null || echo "") if [ -n "$last_tag" ]; then commits=$(git log --oneline "${last_tag}..HEAD" 2>/dev/null || echo "") else commits=$(git log --oneline -20 2>/dev/null || echo "") fi - - # Create release notes entry - local entry="## ${version} - ${date}\n\n" - if [ -n "$commits" ]; then - entry+="### Changes\n\n" - while IFS= read -r line; do - if [ -n "$line" ]; then - entry+="- ${line#* }\n" - fi - done <<< "$commits" - fi - entry+="\n" - - # Insert after the release notes header - if [ -f "$release_notes_file" ]; then - local temp_file=$(mktemp) - head -2 "$release_notes_file" > "$temp_file" - echo -e "$entry" >> "$temp_file" - tail -n +3 "$release_notes_file" >> "$temp_file" - mv "$temp_file" "$release_notes_file" - echo -e "${GREEN}Updated ${release_notes_file}${NC}" - fi + rc=0 + insert_release_section "$version" "$date" "$commits" "$release_notes_file" || rc=$? + case $rc in + 0) echo -e "${GREEN}Updated ${release_notes_file} from commit log${NC}" ;; + 2) echo -e "${YELLOW}${release_notes_file} already has notes for ${version}${NC}" ;; + *) echo -e "${RED}Failed to update ${release_notes_file}${NC}" >&2; return 1 ;; + esac } # Publish a subproject @@ -296,12 +304,25 @@ echo "" CURRENT_VERSION=$(get_version) echo -e "Current version: ${YELLOW}${CURRENT_VERSION}${NC}" +if ! validate_version "$CURRENT_VERSION"; then + echo -e "${RED}Aborting: build.gradle version '${CURRENT_VERSION}' is not MAJOR.MINOR.PATCH[-SNAPSHOT].${NC}" >&2 + exit 1 +fi commit_release_version() { local release_version=$1 + local file + for file in build.gradle README.md release.md; do + if ! require_clean_release_file "$file"; then + exit 1 + fi + done + if ! generate_release_notes "$release_version"; then + echo -e "${RED}Release notes failed; build.gradle and README.md were not changed. Inspect release.md before retrying.${NC}" >&2 + exit 1 + fi update_version "$release_version" update_readme_version "$release_version" - generate_release_notes "$release_version" if ! git add build.gradle README.md release.md; then echo -e "${RED}Error: Failed to add files to git. Please resolve the issue and try again.${NC}" >&2 @@ -318,6 +339,10 @@ commit_release_version() { # then publishing a different version. if [ -n "$BUMP_TYPE" ]; then RELEASE_VERSION=$(bump_version "$CURRENT_VERSION" "$BUMP_TYPE") + if ! validate_version "$RELEASE_VERSION"; then + echo -e "${RED}Aborting: bumped version '${RELEASE_VERSION}' is malformed.${NC}" >&2 + exit 1 + fi echo -e "Bumping version to: ${GREEN}${RELEASE_VERSION}${NC}" elif echo "$CURRENT_VERSION" | grep -q '\-SNAPSHOT'; then RELEASE_VERSION="${CURRENT_VERSION%-SNAPSHOT}" @@ -333,12 +358,12 @@ CURRENT_VERSION="$RELEASE_VERSION" # Check if version has already been released (git tag exists) TAG="v${CURRENT_VERSION}" -if git rev-parse "$TAG" >/dev/null 2>&1 || git ls-remote --tags origin | grep -q "refs/tags/$TAG$"; then +if git rev-parse "$TAG" >/dev/null 2>&1 || git ls-remote --tags origin 2>/dev/null | grep -q "refs/tags/$TAG$"; then if [ "$DRY_RUN" = true ]; then echo -e "${YELLOW}Warning: Tag $TAG already exists. This version may have already been released.${NC}" else echo -e "${RED}Warning: Version ${CURRENT_VERSION} appears to have already been released (tag $TAG exists).${NC}" - read -p "Continue anyway? [y/N]: " confirm + read -r -p "Continue anyway? [y/N]: " confirm if [[ ! "$confirm" =~ ^[Yy]$ ]]; then echo -e "${RED}Aborting release.${NC}" exit 1 @@ -350,27 +375,49 @@ fi if [ "$DRY_RUN" = false ] && [ "$RELEASE_VERSION" != "$(get_version)" ]; then commit_release_version "$RELEASE_VERSION" elif [ "$DRY_RUN" = true ] && [ "$RELEASE_VERSION" != "$(get_version)" ]; then - echo -e "${YELLOW}[DRY RUN] Would update build.gradle and README.md to ${RELEASE_VERSION}${NC}" -elif [ "$README_NEEDS_UPDATE" = true ]; then + for file in build.gradle README.md release.md; do + if ! require_clean_release_file "$file"; then + echo -e "${YELLOW}[DRY RUN] A real release would stop before changing ${file}.${NC}" >&2 + fi + done + echo -e "${YELLOW}[DRY RUN] Would update build.gradle and README.md to ${RELEASE_VERSION}, and release.md for ${RELEASE_VERSION}${NC}" +else if [ "$DRY_RUN" = true ]; then - echo -e "${YELLOW}[DRY RUN] Would update README.md to match version ${CURRENT_VERSION}${NC}" + if ! require_clean_release_file release.md; then + echo -e "${YELLOW}[DRY RUN] A real release would stop before changing release.md.${NC}" >&2 + fi + echo -e "${YELLOW}[DRY RUN] Would update release.md for ${CURRENT_VERSION}${NC}" + if [ "$README_NEEDS_UPDATE" = true ]; then + echo -e "${YELLOW}[DRY RUN] Would update README.md to match version ${CURRENT_VERSION}${NC}" + if ! require_clean_release_file README.md; then + echo -e "${YELLOW}[DRY RUN] Accepting the README.md update would stop the release; declining it would continue.${NC}" >&2 + fi + fi else - read -p "Update README.md to version ${CURRENT_VERSION}? [Y/n]: " update_readme - if [[ ! "$update_readme" =~ ^[Nn]$ ]]; then - update_readme_version "$CURRENT_VERSION" - if ! git diff --quiet HEAD -- README.md; then - if ! git add README.md; then - echo -e "${RED}Error: Failed to add README.md to git. Please resolve the issue and try again.${NC}" >&2 - exit 1 - fi - if ! git commit -m "Update README version to ${CURRENT_VERSION}" -- README.md; then - echo -e "${RED}Error: Failed to commit README.md version change. Please resolve the issue and try again.${NC}" >&2 + update_readme=false + readme_reply='' + if ! require_clean_release_file release.md; then + exit 1 + fi + if [ "$README_NEEDS_UPDATE" = true ]; then + read -r -p "Update README.md to version ${CURRENT_VERSION}? [Y/n]: " readme_reply + if [[ ! "$readme_reply" =~ ^[Nn]$ ]]; then + if ! require_clean_release_file README.md; then exit 1 fi - else - echo -e "${YELLOW}README.md already matches version ${CURRENT_VERSION}; no commit needed.${NC}" + update_readme=true fi fi + if ! generate_release_notes "$CURRENT_VERSION"; then + echo -e "${RED}Release notes failed; README.md was not changed. Inspect release.md before retrying.${NC}" >&2 + exit 1 + fi + if [ "$update_readme" = true ]; then + update_readme_version "$CURRENT_VERSION" + fi + if ! commit_changed_release_files "$CURRENT_VERSION" "$update_readme"; then + exit 1 + fi fi fi echo "" diff --git a/req/v0.4.1-fixes.md b/req/v0.4.1-fixes.md new file mode 100644 index 0000000..0350583 --- /dev/null +++ b/req/v0.4.1-fixes.md @@ -0,0 +1,2645 @@ +# Code Review Fixes (v0.4.1, 2026-09-26) Implementation Plan + +> **Status:** Implemented. This is the historical plan; the checked steps record completed v0.4.1 work. Current behavior is documented in the source and project guides. + +**Goal:** Fix the 15 defects found in the 2026-09-26 review of GuiInteraction — one that silently changes the bytecode level of every published artifact, two that crash on a null argument, one that corrupts the changelog on the next release, and eleven build/CI/packaging/process gaps — without changing the public API shape. + +**Architecture:** Runtime fixes follow the pattern established by the 2026-09-25 plan: the faulty expression is extracted into a small package-scope static helper on the class that owns it, and that helper is unit-tested directly. This is required, not stylistic — `gi-swing` and `gi-fx` run their tests with `java.awt.headless=true` and both `InOut` constructors throw `UnsupportedOperationException` when headless, so no instance method of those classes is reachable from a test. `gi-common` and `gi-console` fixes are tested through their existing instance-level test classes. Build fixes are verified by running the affected Gradle task and inspecting its output; the shell-script fix is extracted into a sourceable library so it can be exercised against a fixture. + +**Tech Stack:** Groovy 5.1.3 (`@CompileStatic`), Java 21 toolchain, Gradle 9.6.1 multi-module, JUnit Jupiter 6.1.3, Apache Tika core 3.3.2, JavaFX 23.0.2 (compileOnly), GradleUp Shadow 9.6.1, Spotless 8.10.3, SpotBugs plugin 6.5.12 / engine 4.10.4, OWASP dependency-check 13.0.0. + +**Spec:** The review findings are reproduced in the *Findings Index* below. There is no separate spec document; this plan and that index travel together. + +## Findings Index + +Every task names the finding numbers it closes. Follow-up review fixes are tracked in the current source and documentation, including `checkCVE.sh` as the fresh aggregate CVE scan entry point. + +| # | Severity | Finding | Task | +|---|----------|---------|------| +| 1 | **High** | The Java 21 toolchain is never applied to any subproject; modules compile at the Gradle JVM's level | 1 | +| 2 | **High** | `gi-console` `display(String)` throws `NullPointerException` for a null file name | 5 | +| 3 | **High** | `release.sh` inserts generated notes *above* `## Unreleased`, so the released notes are wrong and the curated section is never promoted | 12 | +| 4 | Medium | The OWASP CVE scan scans nothing (`dependencyCheckAnalyze` on the dependency-free root) and CI's `dependency-check` job runs `dependencyUpdates` instead | 3 | +| 5 | Medium | `./gradlew check` and `./gradlew build` skip Spotless and SpotBugs entirely with the repo's default settings | 2 | +| 6 | Medium | `gi-console` `promptPassword` throws `NullPointerException` at EOF instead of returning null | 6 | +| 7 | Medium | The JavaFX platform classifier assumes aarch64 on macOS and yields an unresolvable `unknown` elsewhere | 4 | +| 8 | Medium | `view(Matrix)` / `Viewer.viewTable(Matrix)` NPE on a null matrix; an unnamed matrix renders the tab title `null (3 rows)` | 7 | +| 9 | Low | `gi-fx` `rowsForViewer` is an identity function with a test that asserts only identity | 8 | +| 10 | Low | Fat JARs contain 8 duplicated `LICENSE`/`NOTICE`/`DEPENDENCIES`/`buildinfo.xml` entries | 11 | +| 11 | Low | `Viewer.KEY_CODE_COPY` uses a locale-sensitive `toLowerCase()` for the OS check | 7 | +| 12 | Low | `release.sh` mangles commit subjects through `echo -e` and does not validate the version shape before bumping | 12, 13 | +| 13 | Low | `urlExists` swallows every failure with no diagnostics | 10 | +| 14 | Low | CI is Linux-only; the Windows `cmd.exe` branch of `shellCommand` is never exercised | 9 | +| 15 | Low | Docs drift: `DEPENDENCIES.md` documents the wrong CVE task and a version table that stops at 0.2.x | 1, 3, 14 | + +### Evidence gathered during planning + +All of the following were reproduced on `main` at `b8c5179` before this plan was written. + +**Finding 1** — an init-script probe over the configured model: + +``` +PROBE :gi-common toolchain=null PROBE :gi-console toolchain=null +PROBE :gi-fx toolchain=null PROBE :gi-swing toolchain=null +``` + +`subprojects { if (it.plugins.hasPlugin('java')) { ... } }` runs while the *root* project is being configured, before any module script has applied the `groovy`/`java-library` plugins, so the guard is always false. + +**Finding 2** — `new se.alipsa.gi.console.InOut().display((String) null)` → `NullPointerException`, while `gi-swing` and `gi-fx` log and return. + +**Finding 4** — `./gradlew -q dependencies --configuration runtimeClasspath` on the root project prints `No dependencies`. The plugin is applied to the root only, and `dependencyCheckAnalyze` is a single-project task. + +**Finding 5** — `./gradlew check` runs 0 tasks whose name contains `spotless` or `spotbugs`; `check --dry-run` lists none. Removing the `enableSpotless`/`enableSpotbugs` guards *and* both `notCompatibleWithConfigurationCache` markers and running `./gradlew spotlessCheck spotbugsMain --rerun-tasks` with the configuration cache enabled ends in `BUILD SUCCESSFUL` and `Configuration cache entry stored.` — the markers are obsolete for Spotless 8.10.3 / SpotBugs 6.5.12 on Gradle 9.6.1. + +**Finding 8** — `Matrix.builder().rows([[1, 2]]).build().matrixName` is `null`. + +**Finding 10** — `unzip -l gi-console-0.4.1-SNAPSHOT-fatjar.jar | awk 'NF>=4{print $4}' | sort | uniq -d` lists `LICENSE`, `META-INF/buildinfo.xml`, `META-INF/DEPENDENCIES`, `META-INF/LICENSE`, `META-INF/LICENSE.txt`, `META-INF/NOTICE`, `META-INF/NOTICE.txt`, `NOTICE`. The transformer configuration in Task 11 was applied experimentally and reduces that list to empty while keeping every licence text and the merged service/extension-module files. + +**Finding 13/14** — `gi-fx`'s `Viewer` class initialises and its pure static helpers run under `java.awt.headless=true` (`TableDataTest` already relies on this), so the new `Viewer` helper in Task 7 is testable. + +## Global Constraints + +- **Java 21 toolchain, Groovy first, `@CompileStatic` where practical.** New code follows the file it lives in. +- **Indentation: 2 spaces, UTF-8** — except `gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy` and `gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy`, which use 4 spaces. Match the surrounding file, not the repo default, in those two. +- **Naming:** classes `PascalCase`, methods/fields `camelCase`, constants `UPPER_SNAKE_CASE`. +- **Tests:** JUnit Jupiter, under `/src/test/groovy`, named `*Test.groovy`. Add to the existing test class for the module unless a task says otherwise. +- **`./gradlew test` must pass after every task**, and `./gradlew check --no-configuration-cache` must pass after every task. Both are green on `main` today (verified 2026-09-26), so any failure is yours. +- **Do not invoke system Gradle.** Always `./gradlew` from the repository root. +- **AGENTS.md rule: never commit unless explicitly instructed.** Each task ends with a commit step. Execute it only if the human partner authorised commits when they chose an execution method; otherwise stop after the verification step and report. +- **Commit messages:** short imperative, matching existing history (`fix shell command execution API`, `improve display methods`). End every commit message with: + ``` + Co-Authored-By: Claude Opus 5 + ``` +- **A task is not done until implementation, tests, and documentation are done** (AGENTS.md). Documentation steps are folded into the task that needs them. +- **No public API signature changes.** Every fix preserves the signatures declared in `se.alipsa.gi.GuiInteraction`. +- **Tasks 1, 2 and 3 all modify the root `build.gradle`; Tasks 12 and 13 both modify `release.sh` and `release-lib.sh`; Tasks 9 and 10 both modify `gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy`.** Run each of those groups in ascending order; do not parallelise within a group. +- **`test.gsh` sample scripts pin the latest *released* version (0.4.0).** Do not bump them in this plan (AGENTS.md). + +## Review Focus + +Input classes these findings imply that no fix naturally exercises. Each has a test assigned to the task that owns the code. + +1. **`promptPassword` at EOF with no system console** — the fallback branch reads `sysin`, which returns `null`; a caller who pipes an empty stdin must get `null`, not a crash or an empty password. → Task 6. +2. **A `Matrix` built without a name** — `matrixName` is null, and the JavaFX tab title is built by string concatenation, so it renders `null (3 rows)` rather than a usable label. → Task 7. +3. **`shellCommand` on Windows with `ComSpec` unset** — the fallback to `cmd.exe` is the only thing standing between a stripped environment and a `NullPointerException` in `ProcessBuilder`. → Task 9. +4. **A release re-run for a version that was already written to the changelog** — `release.sh --skip` exists to resume a partial publish with the same version, and promotion leaves a fresh empty `## Unreleased` behind, so a second run must not promote that empty section into a duplicate heading. → Task 12. +5. **`release.md` with no `## Unreleased` section** — a release run straight after someone hand-edits the changelog must still produce a section, not silently write nothing. → Task 12. +6. **A version bash mishandles when bumping** — `bump_version` computes on the version fields, so a two-component version (`0.4`) bumps *silently* to a plausible wrong answer, because an empty field is `0` inside `$(( ))`, while a leading zero (`0.08.1`, invalid octal) aborts the run outright. Both land after the tag check has passed. → Task 13. + +--- + +## Task 1: Apply the Java 21 toolchain to every subproject + +**Closes:** Findings 1, 15 (version table) + +The root `build.gradle` intends to force a Java 21 toolchain on every module. The guard it uses can never be true, so all four published modules compile with whatever JDK runs the Gradle daemon. On a JDK 25 daemon the project publishes class-file major 69 artifacts that no Java 21 consumer can load, while README and AGENTS.md promise "Any JDK 21+". + +**Files:** +- Modify: `build.gradle:32-41` (the head of the `subprojects` block) +- Modify: `docs/DEPENDENCIES.md` (the *Version Compatibility* table) +- Create (scratch, not committed): `/tmp/toolchain-probe.gradle` + +**Interfaces:** +- Consumes: nothing. +- Produces: nothing at the code level. Later tasks are unaffected. + +- [x] **Step 1: Write the failing check** + +Create `/tmp/toolchain-probe.gradle`: + +```groovy +allprojects { p -> + p.afterEvaluate { + def je = p.extensions.findByName('java') + if (je != null) { + println "PROBE ${p.path} toolchain=${je.toolchain.languageVersion.getOrNull()}" + } + } +} +``` + +- [x] **Step 2: Run the probe to see the bug** + +Run: +```bash +./gradlew -I /tmp/toolchain-probe.gradle -q help --no-configuration-cache 2>/dev/null | grep PROBE +``` +Expected (this is the bug): every subproject line reads `toolchain=null`. + +``` +PROBE :gi-common toolchain=null +PROBE :gi-console toolchain=null +PROBE :gi-fx toolchain=null +PROBE :gi-swing toolchain=null +``` + +- [x] **Step 3: React to the plugin instead of polling for it** + +In `build.gradle`, replace: + +```groovy +subprojects { + group = rootProject.group + version = rootProject.version + if (it.plugins.hasPlugin('java')) { + java { + toolchain { + languageVersion.set(JavaLanguageVersion.of(21)) + } + } + } +``` + +with: + +```groovy +subprojects { + group = rootProject.group + version = rootProject.version + // pluginManager.withPlugin runs when (and if) the module applies the plugin. + // A plugins.hasPlugin() check here is always false: this block is evaluated + // while the root project is configured, before any module script has run, so + // the toolchain was silently never applied and modules compiled at the + // Gradle JVM's language level. + pluginManager.withPlugin('java') { + java { + toolchain { + languageVersion.set(JavaLanguageVersion.of(21)) + } + } + } +``` + +- [x] **Step 4: Run the probe again to verify the fix** + +Run: +```bash +./gradlew -I /tmp/toolchain-probe.gradle -q help --no-configuration-cache 2>/dev/null | grep PROBE +``` +Expected: every subproject line now reads `toolchain=21`. + +``` +PROBE :gi-common toolchain=21 +PROBE :gi-console toolchain=21 +PROBE :gi-fx toolchain=21 +PROBE :gi-swing toolchain=21 +``` + +- [x] **Step 5: Verify the build still compiles and that the bytecode level is pinned** + +Run: +```bash +./gradlew clean build --no-configuration-cache --console=plain +od -An -t u1 -j 6 -N 2 gi-common/build/classes/groovy/main/se/alipsa/gi/FileUtils.class | awk '{print $1*256+$2}' +``` +Expected: `BUILD SUCCESSFUL`, and the class-file major version prints `65` (Java 21). + +Note: Gradle satisfies the toolchain from the JDK that runs the daemon when its version matches. No toolchain-download resolver is configured, so a contributor on a non-21 JDK must have a Java 21 installation discoverable by Gradle; that is the intended behaviour and matches `release.sh`, which selects a JDK 21 through SDKMAN. + +- [x] **Step 6: Refresh the stale version-compatibility table** + +In `docs/DEPENDENCIES.md`, replace: + +```markdown +| GuiInteraction | Java | Groovy | JavaFX | +|----------------|------|--------|--------| +| 0.2.x | 21+ | 5.0.x | 21-23 | +| 0.1.x | 17+ | 4.0.x | 17-21 | +``` + +with: + +```markdown +| GuiInteraction | Java | Groovy | JavaFX | +|----------------|------|--------|--------| +| 0.4.x | 21 | 5.1.x | 23 | +| 0.3.x | 21+ | 5.0.x | 21-23 | +| 0.2.x | 21+ | 5.0.x | 21-23 | +| 0.1.x | 17+ | 4.0.x | 17-21 | + +Every module is compiled with a Java 21 toolchain, configured once in the root +`build.gradle` and applied to each module through `pluginManager.withPlugin('java')`. +The toolchain fixes the bytecode level regardless of which JDK runs Gradle, so the +published artifacts load on any Java 21 or later runtime. +``` + +- [x] **Step 7: Commit** + +```bash +git add build.gradle docs/DEPENDENCIES.md +git commit -m "apply the java 21 toolchain to every subproject + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 2: Make Spotless and SpotBugs part of the default check + +**Closes:** Finding 5 + +`gradle.properties` enables the configuration cache, which makes `configurationCacheRequested` true, which makes `enableSpotless` and `enableSpotbugs` false unless the requested task name happens to contain "spotless" or "spotbugs". The result is that `./gradlew check` and `./gradlew build` run no static analysis at all, silently, for every contributor and for CI's first build step. The `notCompatibleWithConfigurationCache` markers those flags exist to work around are obsolete: Spotless 8.10.3 and the SpotBugs plugin 6.5.12 both run under the Gradle 9.6.1 configuration cache (verified during planning). + +**Files:** +- Modify: `build.gradle:21-24` (the enable flags) and `build.gradle:43-89` (the `subprojects` analysis blocks) +- Modify: `.github/workflows/ci.yml:33-37` +- Modify: `AGENTS.md` (Build, Test, and Development Commands) + +**Interfaces:** +- Consumes: the `subprojects` block as Task 1 left it. +- Produces: `check` now depends on `spotlessCheck`, `spotbugsMain` and `spotbugsTest` in every module. + +- [x] **Step 1: Confirm the bug** + +Run: +```bash +./gradlew check --dry-run | grep -ciE 'spotless|spotbugs' +``` +Expected (this is the bug): `0`. + +- [x] **Step 2: Delete the enable flags** + +In `build.gradle`, delete these four lines: + +```groovy +def requestedTaskNames = gradle.startParameter.taskNames.collect { it.toLowerCase(Locale.ROOT) } +def configurationCacheRequested = gradle.startParameter.configurationCacheRequested +def enableSpotless = !configurationCacheRequested || requestedTaskNames.any { it.contains('spotless') } +def enableSpotbugs = !configurationCacheRequested || requestedTaskNames.any { it.contains('spotbugs') } +``` + +- [x] **Step 3: Apply both plugins unconditionally** + +In the `subprojects` block, replace everything from `if (enableSpotless) {` through the closing brace of `if (enableSpotbugs) { ... }` with: + +```groovy + // Spotless 8.x and the SpotBugs plugin 6.x both support the Gradle + // configuration cache, so these run in the default `check` — verified with + // `./gradlew spotlessCheck spotbugsMain --rerun-tasks` on Gradle 9.6.1. + apply plugin: 'com.diffplug.spotless' + spotless { + java { + googleJavaFormat('1.34.0') + } + groovy { + excludeJava() + leadingTabsToSpaces(2) + trimTrailingWhitespace() + endWithNewline() + } + groovyGradle { + leadingTabsToSpaces(2) + trimTrailingWhitespace() + endWithNewline() + } + } + + apply plugin: 'com.github.spotbugs' + spotbugs { + toolVersion = '4.10.4' + effort = 'max' + reportLevel = 'medium' + excludeFilter = rootProject.file('config/spotbugs-exclude.xml') + } + tasks.withType(com.github.spotbugs.snom.SpotBugsTask).configureEach { spotbugsTask -> + String suffix = spotbugsTask.name.startsWith('spotbugs') ? + spotbugsTask.name.substring('spotbugs'.length()) : '' + String sourceSetName = suffix.isEmpty() ? 'main' : + suffix.substring(0, 1).toLowerCase(Locale.ROOT) + suffix.substring(1) + def sourceSet = project.sourceSets.findByName(sourceSetName) + if (sourceSet != null) { + auxClassPaths.from(sourceSet.compileClasspath, sourceSet.runtimeClasspath) + } + reports { + html.required = true + xml.required = false + } + } +``` + +- [x] **Step 4: Verify analysis now runs with the configuration cache on** + +Run: +```bash +./gradlew check --dry-run | grep -ciE 'spotless|spotbugs' +./gradlew spotlessCheck spotbugsMain --rerun-tasks +``` +Expected: the first command prints a number greater than `20`; the second ends in `BUILD SUCCESSFUL` followed by `Configuration cache entry stored.` (it takes roughly two minutes — SpotBugs at `effort = max` over four modules). + +- [x] **Step 5: Verify the full default build is still green** + +Run: `./gradlew clean build` +Expected: `BUILD SUCCESSFUL`, and the log now contains `spotbugsMain` and `spotlessGroovyCheck` tasks. + +- [x] **Step 6: Collapse the redundant CI step** + +In `.github/workflows/ci.yml`, replace: + +```yaml + - name: Build with Gradle + run: ./gradlew build + + - name: Verify with Spotless and SpotBugs + run: ./gradlew check --no-configuration-cache +``` + +with: + +```yaml + - name: Build with Gradle (runs tests, Spotless and SpotBugs) + run: ./gradlew build +``` + +`check.sh` now runs `release-lib-test.sh`, `spotlessApply`, and `check` with the default configuration-cache setting. + +- [x] **Step 7: Document the command** + +In `AGENTS.md`, under *Build, Test, and Development Commands*, replace the `./gradlew build` bullet with: + +```markdown +- `./gradlew build` — compile all modules, run tests, and run Spotless and SpotBugs (static analysis is part of `check`, and `check` is part of `build`). +- `./check.sh` — reformat with Spotless and then run the full `check`; use this before pushing. +``` + +- [x] **Step 8: Commit** + +```bash +git add build.gradle .github/workflows/ci.yml AGENTS.md +git commit -m "run spotless and spotbugs as part of the default check + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 3: Make the OWASP scan scan the modules + +**Closes:** Findings 4, 15 (CVE task name) + +The dependency-check plugin is applied to the root project only, and the root declares no dependencies — `./gradlew -q dependencies --configuration runtimeClasspath` prints `No dependencies`. `dependencyCheckAnalyze` is a single-project task, so `failBuildOnCVSS = 7.0` currently guards an empty set. The CI job named `dependency-check` compounds this by running `dependencyUpdates`, a different task entirely. `dependencyCheckAggregate` is the multi-project task and covers the root plus all four modules. + +**Files:** +- Modify: `build.gradle:133-135` (the comment above the `dependencyCheck` block) +- Modify: `.github/workflows/ci.yml:55-81` (the `dependency-check` job) +- Modify: `docs/DEPENDENCIES.md` (*Security Scanning* and *CI/CD Integration*) + +**Interfaces:** +- Consumes: the root `build.gradle` as Task 2 left it. +- Produces: nothing at the code level. + +- [x] **Step 1: Confirm the scope bug** + +Run: +```bash +./gradlew -q dependencies --configuration runtimeClasspath --no-configuration-cache | head -8 +``` +Expected (this is the bug): `No dependencies` — the only configuration `dependencyCheckAnalyze` would scan is empty. + +- [x] **Step 2: Confirm the aggregate task exists and would run** + +Run: +```bash +./gradlew dependencyCheckAggregate --dry-run --no-configuration-cache | grep -i dependencyCheck +``` +Expected: the dry run lists `:dependencyCheckAggregate` (and `:dependencyCheckUpdate`), proving the task is available without needing an NVD download. + +- [x] **Step 3: Point the in-file instructions at the aggregate task** + +In `build.gradle`, replace: + +```groovy +// OWASP Dependency Check configuration +// Run with: ./gradlew dependencyCheckAnalyze --rerun-tasks --no-configuration-cache --console=plain +// Reports are generated in build/reports/dependency-check/ +``` + +with: + +```groovy +// OWASP Dependency Check configuration +// Run with: ./gradlew dependencyCheckAggregate --rerun-tasks --no-configuration-cache --console=plain +// Use the aggregate task, not dependencyCheckAnalyze: the plugin is applied to the +// root project, which declares no dependencies of its own, so the single-project +// task would scan an empty dependency set and always pass. +// Reports are generated in build/reports/dependency-check/ +``` + +- [x] **Step 4: Make the CI job actually scan** + +In `.github/workflows/ci.yml`, replace the last three steps of the `dependency-check` job: + +```yaml + - name: Check for dependency updates + run: ./gradlew dependencyUpdates + + - name: Upload dependency report + uses: actions/upload-artifact@v4 + with: + name: dependency-updates-report + path: 'build/dependencyUpdates/' + retention-days: 30 +``` + +with: + +```yaml + - name: Scan dependencies for known vulnerabilities + env: + NVD_API_KEY: ${{ secrets.NVD_API_KEY }} + run: ./gradlew dependencyCheckAggregate --no-configuration-cache + + - name: Check for dependency updates + if: always() + run: ./gradlew dependencyUpdates + + - name: Upload dependency-check report + uses: actions/upload-artifact@v4 + if: always() + with: + name: dependency-check-report + path: 'build/reports/dependency-check/' + retention-days: 30 + + - name: Upload dependency report + uses: actions/upload-artifact@v4 + if: always() + with: + name: dependency-updates-report + path: 'build/dependencyUpdates/' + retention-days: 30 +``` + +- [x] **Step 5: Verify the workflow still parses** + +Run: +```bash +python3 -c "import yaml,sys; yaml.safe_load(open('.github/workflows/ci.yml')); print('ci.yml parses')" +``` +Expected: `ci.yml parses`. + +- [x] **Step 6: Fix the documentation** + +In `docs/DEPENDENCIES.md`, replace: + +```bash +# Run the security scan +./gradlew dependencyCheckAnalyze --no-configuration-cache --console=plain +``` + +with: + +```bash +# Run the security scan across the root project and all four modules +./gradlew dependencyCheckAggregate --no-configuration-cache --console=plain +``` + +and add, immediately after that code block: + +```markdown +Use `dependencyCheckAggregate`, not `dependencyCheckAnalyze`. The plugin is applied +to the root project, which has no dependencies of its own, so the single-project +task scans nothing and always passes. +``` + +Then, in the *CI/CD Integration* section, replace: + +```yaml +dependency-check: + runs-on: ubuntu-latest + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + + steps: + - name: Check for dependency updates + run: ./gradlew dependencyUpdates +``` + +with: + +```yaml +dependency-check: + runs-on: ubuntu-latest + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + + steps: + - name: Scan dependencies for known vulnerabilities + env: + NVD_API_KEY: ${{ secrets.NVD_API_KEY }} + run: ./gradlew dependencyCheckAggregate --no-configuration-cache + + - name: Check for dependency updates + if: always() + run: ./gradlew dependencyUpdates +``` + +and replace the sentence below it — "This approach keeps regular CI builds fast while still providing periodic dependency monitoring." — with: + +```markdown +This approach keeps regular CI builds fast while still scanning for CVEs and +tracking dependency drift on a schedule. The scan needs the `NVD_API_KEY` +repository secret; without it the NVD feed download is rate-limited to the point +of timing out. +``` + +- [x] **Step 7: Verify the build is unaffected** + +Run: `./gradlew check` +Expected: `BUILD SUCCESSFUL`. + +A full `dependencyCheckAggregate` run needs an `NVD_API_KEY` and a multi-minute feed download; it is not part of this task's verification. Note for the reviewer: the fact that the aggregate task covers subprojects is the plugin's documented behaviour, confirmed here only as far as the task existing and being schedulable. + +- [x] **Step 8: Commit** + +```bash +git add build.gradle .github/workflows/ci.yml docs/DEPENDENCIES.md +git commit -m "scan every module for CVEs with dependencyCheckAggregate + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 4: Resolve the JavaFX platform classifier correctly + +**Closes:** Finding 7 + +`gi-fx/build.gradle` and `gi-swing/build.gradle` each contain the same copy of an OS switch that maps every macOS to `mac-aarch64` and every other platform to `unknown`. An Intel Mac therefore resolves aarch64 natives (for `gi-fx` these are `testImplementation`, so tests load them), and any other OS produces an unresolvable coordinate with an opaque error instead of a clear message. + +**Files:** +- Create: `buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle` +- Modify: `gi-fx/build.gradle:1-8` (plugins) and `gi-fx/build.gradle:31-43` (dependencies head) +- Modify: `gi-swing/build.gradle:1-9` (plugins) and `gi-swing/build.gradle:28-40` (dependencies head) + +**Interfaces:** +- Consumes: nothing. +- Produces: `project.ext.javafxClassifier` — a `String` holding the OpenJFX classifier for the machine running the build (`linux`, `linux-aarch64`, `win`, `mac`, `mac-aarch64`). Available to any module that applies `se.alipsa.gi.javafx-platform`. + +- [x] **Step 1: Record the current resolution as the baseline** + +Run: +```bash +./gradlew -q :gi-fx:dependencies --configuration testCompileClasspath --no-configuration-cache | grep -m3 javafx +``` +Expected on this Linux machine: coordinates ending in `:linux`. Keep the output; Step 5 must match it. + +- [x] **Step 2: Write the convention plugin** + +Create `buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle`: + +```groovy +/** + * Resolves the OpenJFX platform classifier for the machine running the build and + * exposes it as `javafxClassifier`. + * + * OpenJFX publishes one artifact per platform. Guessing wrong is not a build + * failure but a runtime one: the wrong natives load and fail with + * UnsatisfiedLinkError. macOS ships on both x64 and aarch64, so the architecture + * has to be part of the decision, and an unrecognised platform must fail loudly + * rather than produce an unresolvable 'unknown' coordinate. + */ + +import org.gradle.internal.os.OperatingSystem + +def currentOs = OperatingSystem.current() +def arch = System.getProperty('os.arch', '').toLowerCase(Locale.ROOT) +def aarch64 = arch in ['aarch64', 'arm64'] +def x64 = arch in ['x86_64', 'amd64'] + +def unsupported = { + throw new GradleException( + "Unsupported platform for JavaFX: ${currentOs.name} (os.arch=${arch}). " + + "Supported: Linux x64/aarch64, macOS x64/aarch64, Windows x64.") +} + +// A JVM can only load natives built for its own architecture, and os.arch reports +// the JVM's architecture rather than the machine's. So every branch below picks the +// artifact matching os.arch or fails naming it: substituting the x64 build on an +// aarch64 JVM (or vice versa) is a guaranteed UnsatisfiedLinkError at runtime, never +// something emulation rescues. +String classifier +if (currentOs.linux) { + if (aarch64) { + classifier = 'linux-aarch64' + } else if (x64) { + classifier = 'linux' + } else { + unsupported() + } +} else if (currentOs.macOsX) { + if (aarch64) { + classifier = 'mac-aarch64' + } else if (x64) { + classifier = 'mac' + } else { + unsupported() + } +} else if (currentOs.windows) { + // OpenJFX publishes only an x64 Windows build -- checked against Maven Central for + // 23.0.2, where the classifiers are linux, linux-aarch64, mac, mac-aarch64 and win. + // An Arm64 JVM cannot borrow it. On Windows on Arm an Arm64 process can load only + // Arm64 binaries; x64 binaries load into x64 or Arm64EC processes. The emulator + // runs an x64 *process* -- it does not let an Arm64 process load an x64 DLL. + // See https://learn.microsoft.com/en-us/windows/arm/arm64ec#interoperability + // os.arch reports the JVM's architecture, not the machine's, so the supported + // route on an Arm64 machine is an x64 JDK: that process is emulated and reports + // os.arch=amd64 here, which selects 'win' correctly. + if (!x64) { + throw new GradleException( + "JavaFX publishes no ${arch} Windows build, and a ${arch} JVM cannot load the " + + "x64 natives: on Windows on Arm an Arm64 process can load only Arm64 binaries. " + + "Build with an x64 JDK instead -- it runs under emulation on Arm and reports " + + "os.arch=amd64. See https://learn.microsoft.com/en-us/windows/arm/arm64ec#interoperability") + } + classifier = 'win' +} else { + unsupported() +} + +ext.javafxClassifier = classifier +``` + +- [x] **Step 3: Use it from gi-fx** + +In `gi-fx/build.gradle`, add the plugin id to the `plugins` block: + +```groovy +plugins { + id 'groovy' + id 'java-library' + id 'signing' + id 'maven-publish' + id("se.alipsa.nexus-release-plugin") version '2.2.0' + id 'se.alipsa.gi.fatjar-conventions' + id 'se.alipsa.gi.javafx-platform' +} +``` + +and replace the head of the `dependencies` block: + +```groovy +dependencies { + org.gradle.internal.os.OperatingSystem os = org.gradle.internal.os.OperatingSystem.current() + + // check https://repo1.maven.org/maven2/org/openjfx/javafx/20/javafx-20.pom for the platform property + def qualifier = 'unknown' + if (os.isLinux()) { + qualifier='linux' + } else if (os.isWindows()) { + qualifier = 'win' + } else if (os.isMacOsX()) { + qualifier = 'mac-aarch64' + } + def javaFxVersion = libs.versions.javafx.get() +``` + +with: + +```groovy +dependencies { + // Set by the se.alipsa.gi.javafx-platform convention plugin. + def qualifier = javafxClassifier + def javaFxVersion = libs.versions.javafx.get() +``` + +Leave every `"org.openjfx:javafx-*:${javaFxVersion}:$qualifier"` line unchanged. + +- [x] **Step 4: Use it from gi-swing** + +In `gi-swing/build.gradle`, add `id 'se.alipsa.gi.javafx-platform'` as the last entry of the `plugins` block, and replace the head of the `dependencies` block: + +```groovy +dependencies { + org.gradle.internal.os.OperatingSystem os = org.gradle.internal.os.OperatingSystem.current() + + // check https://repo1.maven.org/maven2/org/openjfx/javafx/20/javafx-20.pom for the platform property + def qualifier = 'unknown' + if (os.isLinux()) { + qualifier='linux' + } else if (os.isWindows()) { + qualifier = 'win' + } else if (os.isMacOsX()) { + qualifier = 'mac-aarch64' + } + def javaFxVersion = libs.versions.javafx.get() +``` + +with: + +```groovy +dependencies { + // Set by the se.alipsa.gi.javafx-platform convention plugin. + def qualifier = javafxClassifier + def javaFxVersion = libs.versions.javafx.get() +``` + +- [x] **Step 5: Verify resolution is unchanged on this platform** + +Run: +```bash +./gradlew -q :gi-fx:dependencies --configuration testCompileClasspath --no-configuration-cache | grep -m3 javafx +./gradlew -q :gi-swing:dependencies --configuration compileClasspath --no-configuration-cache | grep -m3 javafx +``` +Expected: the same `:linux` coordinates as in Step 1, resolved (no `FAILED` markers). + +- [x] **Step 6: Verify the classifier logic itself** + +Run: +```bash +cat > /tmp/javafx-classifier-check.groovy <<'CHECK' +def classify = { String osName, String rawArch -> + String arch = rawArch.toLowerCase(Locale.ROOT) + boolean aarch64 = arch in ['aarch64', 'arm64'] + boolean x64 = arch in ['x86_64', 'amd64'] + if (osName.startsWith('Linux')) { + if (aarch64) return 'linux-aarch64' + if (x64) return 'linux' + throw new IllegalStateException("unsupported: $osName/$arch") + } + if (osName.startsWith('Mac')) { + if (aarch64) return 'mac-aarch64' + if (x64) return 'mac' + throw new IllegalStateException("unsupported: $osName/$arch") + } + // Only x64 Windows natives are published, and an Arm64 JVM cannot load them. + if (osName.startsWith('Windows')) { + if (x64) return 'win' + throw new IllegalStateException("unsupported: $osName/$arch") + } + throw new IllegalStateException("unsupported: $osName/$arch") +} + +[['Linux', 'amd64', 'linux'], ['Linux', 'aarch64', 'linux-aarch64'], + ['Mac OS X', 'x86_64', 'mac'], ['Mac OS X', 'aarch64', 'mac-aarch64'], + ['Windows 11', 'amd64', 'win']].each { osName, arch, expected -> + assert classify(osName, arch) == expected, "$osName/$arch" +} + +[['Linux', 's390x'], ['Linux', 'i386'], ['Mac OS X', 'ppc'], + ['Windows 11', 'aarch64'], ['SunOS', 'sparcv9']].each { osName, arch -> + try { + classify(osName, arch) + assert false, "$osName/$arch should have been rejected" + } catch (IllegalStateException expected) { + } +} +println 'classifier mapping ok' +CHECK +groovy /tmp/javafx-classifier-check.groovy +``` +Expected: `classifier mapping ok`. This mirrors the plugin's decision table — including that an unsupported architecture is rejected rather than quietly handed x64 natives — and is a sanity check of the mapping, not of the plugin wiring, which Step 5 covers. + +- [x] **Step 7: Verify the whole build** + +Run: `./gradlew clean build` +Expected: `BUILD SUCCESSFUL`. + +- [x] **Step 8: Document the platform support** + +In `docs/DEPENDENCIES.md`, append at the end of the file: + +```markdown +## JavaFX platform natives + +OpenJFX publishes one artifact per platform, so `gi-fx` and `gi-swing` pick a +classifier for the machine running the build. The +`se.alipsa.gi.javafx-platform` convention plugin sets `javafxClassifier` to +`linux`, `linux-aarch64`, `mac`, `mac-aarch64` or `win`, chosen from `os.arch` — +which is the architecture of the JVM running Gradle, not of the machine. Any other +architecture fails the build with its architecture named, because a JVM can only +load natives built for its own architecture and substituting a different build is a +guaranteed `UnsatisfiedLinkError`. + +On Windows that means x64 only: OpenJFX publishes no Arm64 Windows build, and an +Arm64 JVM cannot use the x64 one — on Windows on Arm an Arm64 process can load only +Arm64 binaries, while x64 binaries load into x64 or Arm64EC processes +([Microsoft's interoperability table](https://learn.microsoft.com/en-us/windows/arm/arm64ec#interoperability)). +Emulation runs an x64 *process*; it does not let an Arm64 process load an x64 DLL. +To build on an Arm64 Windows machine, use an x64 JDK: that JVM is emulated, reports +`os.arch=amd64`, and gets the `win` natives it can actually load. Building on one architecture and running on +another needs a rebuild: the JavaFX dependencies are `compileOnly` for both +modules, so the consumer supplies matching natives at runtime. +``` + +- [x] **Step 9: Commit** + +```bash +git add buildSrc/src/main/groovy/se.alipsa.gi.javafx-platform.gradle gi-fx/build.gradle gi-swing/build.gradle docs/DEPENDENCIES.md +git commit -m "resolve the javafx platform classifier per os and architecture + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 5: Stop gi-console display(String) from throwing on null + +**Closes:** Finding 2 + +`display(String fileName)` calls `new File(fileName)`, which throws `NullPointerException` before the null guard inside `display(File)` is ever reached. `gi-swing` and `gi-fx` log and return for the same input, and `docs/API-Guide.md` states that a null file is reported on stdout rather than thrown. This is the deferred TODO item "Define the intended null-handling behavior for console `display(String)`" — the behaviour is now defined as: identical to `display((File) null)`. + +**Files:** +- Modify: `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy:301-304` +- Test: `gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy` +- Modify: `docs/API-Guide.md` (the *gi-console* bullet list) +- Modify: `TODO.md` (the *Deferred* list) + +**Interfaces:** +- Consumes: nothing. +- Produces: nothing. `void display(String fileName, String... title)` keeps its signature. + +- [x] **Step 1: Write the failing test** + +In `ConsolePromptTest.groovy`, next to `displayingANullFileReportsItWithoutThrowing`, add: + +```groovy + @Test + void displayingANullFileNameReportsItWithoutThrowing() { + InOut inOut = new InOut() + + assertTrue(captureStdout { inOut.display((String) null) }.contains('does not exist')) + } +``` + +- [x] **Step 2: Run it to make sure it fails** + +Run: `./gradlew :gi-console:test --tests '*ConsolePromptTest.displayingANullFileNameReportsItWithoutThrowing'` +Expected: FAIL with `NullPointerException` thrown from `new File(String)`. + +- [x] **Step 3: Delegate the null to the File overload** + +In `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy`, replace: + +```groovy + @Override + void display(String fileName, String... title) { + display(new File(fileName), title) + } +``` + +with: + +```groovy + /** + * Displays a file by name. A null name is reported the same way a missing file is: + * new File(null) would throw NullPointerException before display(File) could apply + * its own guard, which the Swing and JavaFX implementations do not do. + */ + @Override + void display(String fileName, String... title) { + display(fileName == null ? (File) null : new File(fileName), title) + } +``` + +- [x] **Step 4: Run the test to verify it passes** + +Run: `./gradlew :gi-console:test --tests '*ConsolePromptTest'` +Expected: PASS, all tests in the class green. + +- [x] **Step 5: Pin the sibling behaviour in gi-common** + +`gi-swing` and `gi-fx` reach the same input through `FileUtils.getResourceUrl(fileName)`, which must keep returning null rather than throwing. Add to `gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy`: + +```groovy + @Test + void aNullOrEmptyResourceNameResolvesToNullInsteadOfThrowing() { + assertNull(FileUtils.getResourceUrl(null)) + assertNull(FileUtils.getResourceUrl('')) + } +``` + +If `assertNull` is not already statically imported in that file, add `import static org.junit.jupiter.api.Assertions.assertNull`. + +- [x] **Step 6: Run both modules' tests** + +Run: `./gradlew :gi-console:test :gi-common:test` +Expected: `BUILD SUCCESSFUL`. + +- [x] **Step 7: Document the behaviour** + +In `docs/API-Guide.md`, in the *gi-console* bullet list, replace: + +```markdown +- `display(File)` opens the file with the system's default application when the platform supports the Desktop OPEN action; otherwise it prints a message. A missing file, a null file, and a failed open are all reported on stdout rather than thrown. +``` + +with: + +```markdown +- `display(File)` opens the file with the system's default application when the platform supports the Desktop OPEN action; otherwise it prints a message. A missing file, a null file, a null file *name*, and a failed open are all reported on stdout rather than thrown, matching `gi-swing` and `gi-fx`. +``` + +In `TODO.md`, under *Deferred*, replace: + +```markdown +- [x] Define the intended null-handling behavior for console `display(String)`. +``` + +with: + +```markdown +- [x] Define the intended null-handling behavior for console `display(String)` — a null name behaves exactly like `display((File) null)`. +``` + +- [x] **Step 8: Commit** + +```bash +git add gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy gi-common/src/test/groovy/se/alipsa/gi/FileUtilsTest.groovy docs/API-Guide.md TODO.md +git commit -m "report a null file name instead of throwing in the console display + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 6: Treat EOF as cancellation in the console password prompt + +**Closes:** Finding 6 + +`Console.readPassword` returns `null` at end of stream (Ctrl-D, a closed or empty stdin). `new String(ch)` then throws `NullPointerException`, while every other console prompt returns `null` for the same input — `ConsolePromptTest.eofIsCancellationForPrompts` already pins that contract for the others. + +**Files:** +- Modify: `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy:197-211` +- Test: `gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy` +- Modify: `docs/API-Guide.md` (the *gi-console* bullet list) + +**Interfaces:** +- Consumes: nothing. +- Produces: `@PackageScope static String toPassword(char[] password)` on `se.alipsa.gi.console.InOut` — returns `null` for a null array, otherwise the array as a `String`. + +- [x] **Step 1: Write the failing tests** + +In `ConsolePromptTest.groovy`, add: + +```groovy + @Test + void anEofPasswordIsCancellationRatherThanACrash() { + assertNull(InOut.toPassword(null)) + assertEquals('s3cret', InOut.toPassword('s3cret'.toCharArray())) + } + + @Test + void theVisiblePasswordFallbackTreatsEofAsCancellation() { + // The fallback is only reachable when no system console is attached, which is + // the case under the Gradle test runner but not in an interactive shell. + Assumptions.assumeTrue(System.console() == null) + InOut inOut = new InOut() + inOut.sysin = new BufferedReader(new StringReader('')) + + assertNull(inOut.promptPassword('title', 'password')) + } +``` + +Add the import `import org.junit.jupiter.api.Assumptions` to the file. + +- [x] **Step 2: Run them to make sure the first one fails** + +Run: `./gradlew :gi-console:test --tests '*ConsolePromptTest.anEofPasswordIsCancellationRatherThanACrash'` +Expected: FAIL — compilation error, `toPassword` does not exist (`[Static type checking] - Cannot find matching method`). + +- [x] **Step 3: Add the helper and use it** + +In `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy`, replace: + +```groovy + @Override + String promptPassword(String title, String message) { + println title + def console = System.console() + if (console != null) { + char[] ch = console.readPassword("$message : ") + return new String(ch) + } +``` + +with: + +```groovy + /** + * Console.readPassword returns null at end of stream (Ctrl-D, empty or closed + * stdin), which every other console prompt reports as cancellation. Passing that + * null to new String(char[]) would throw NullPointerException instead. + */ + @PackageScope + static String toPassword(char[] password) { + return password == null ? null : new String(password) + } + + @Override + String promptPassword(String title, String message) { + println title + def console = System.console() + if (console != null) { + return toPassword(console.readPassword("$message : ")) + } +``` + +Leave the visible-input fallback below it unchanged — `sysin.readLine()` already returns `null` at EOF. + +- [x] **Step 4: Run the tests to verify they pass** + +Run: `./gradlew :gi-console:test --tests '*ConsolePromptTest'` +Expected: PASS. + +- [x] **Step 5: Document it** + +In `docs/API-Guide.md`, in the *gi-console* bullet list, replace: + +```markdown +- Password input is masked when `System.console()` is available; otherwise stdin input is visible and a warning is logged +``` + +with: + +```markdown +- Password input is masked when `System.console()` is available; otherwise stdin input is visible and a warning is logged. End of input (Ctrl-D, an empty piped stdin) is cancellation and returns `null` on both paths. +``` + +- [x] **Step 6: Commit** + +```bash +git add gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy docs/API-Guide.md +git commit -m "treat end of input as cancellation in the console password prompt + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 7: Harden the JavaFX table viewer against null matrices and unnamed ones + +**Closes:** Findings 8, 11 + +`Viewer.viewTable(Matrix)` dereferences `tableMatrix.matrixName` with no guard, so `io.view((Matrix) null)` is a `NullPointerException` — while the sibling `Viewer.viewHtml` does guard its argument. `Matrix.builder()` leaves `matrixName` null unless it is set, so an unnamed matrix renders the tab title `null (3 rows)`. The same file decides the copy shortcut with a locale-sensitive `toLowerCase()`, which `AbstractInOut.isWindows` correctly avoids with `Locale.ROOT`. The console and Swing implementations get the same null guard so all three behave alike. + +**Files:** +- Modify: `gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy:58-61` and `:196-199` (4-space indentation) +- Modify: `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy:261-264` +- Modify: `gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy:323-342` +- Test: `gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy` +- Test: `gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy` +- Modify: `docs/API-Guide.md` (*gi-fx* and *gi-console* bullet lists) + +**Interfaces:** +- Consumes: nothing. +- Produces: `@PackageScope static String matrixTitle(Matrix matrix, String... title)` on `se.alipsa.gi.fx.Viewer` — an explicit non-blank title wins, then the matrix's own name, then the literal `Table`. + +- [x] **Step 1: Write the failing tests** + +In `gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy`, add these imports: + +```groovy +import se.alipsa.matrix.core.Matrix + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow +``` + +and these tests: + +```groovy + @Test + void anUnnamedMatrixGetsAUsableTitleRatherThanTheStringNull() { + Matrix unnamed = Matrix.builder().rows([[1, 2]]).build() + + assertEquals('Table', Viewer.matrixTitle(unnamed)) + assertEquals('Table', Viewer.matrixTitle(null)) + } + + @Test + void anExplicitTitleWinsOverTheMatrixName() { + Matrix named = Matrix.builder().matrixName('from matrix').rows([[1, 2]]).build() + + assertEquals('chosen', Viewer.matrixTitle(named, 'chosen')) + assertEquals('from matrix', Viewer.matrixTitle(named)) + assertEquals('from matrix', Viewer.matrixTitle(named, ' ')) + } + + @Test + void viewingANullMatrixIsIgnoredRatherThanThrowing() { + assertDoesNotThrow({ Viewer.viewTable((Matrix) null) } as org.junit.jupiter.api.function.Executable) + } +``` + +In `gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy`, add: + +```groovy + @Test + void viewingANullMatrixReportsItWithoutThrowing() { + InOut inOut = new InOut() + + assertTrue(captureStdout { inOut.view((se.alipsa.matrix.core.Matrix) null) }.contains('null')) + } +``` + +- [x] **Step 2: Run them to make sure they fail** + +Run: +```bash +./gradlew :gi-fx:test --tests '*TableDataTest' +./gradlew :gi-console:test --tests '*ConsolePromptTest.viewingANullMatrixReportsItWithoutThrowing' +``` +Expected: the gi-fx run fails to compile (`Cannot find matching method ... matrixTitle`); the gi-console run fails with `NullPointerException ... because "tableMatrix" is null`. + +- [x] **Step 3: Add the title helper and the guard to Viewer (4-space indentation)** + +In `gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy`, replace: + +```groovy + static void viewTable(Matrix tableMatrix, String... title) { + String tit = title.length > 0 ? title[0] : tableMatrix.matrixName + viewTable(tableMatrix.columnNames(), tableMatrix.rowList(), tableMatrix.typeNames(), tit) + } +``` + +with: + +```groovy + static void viewTable(Matrix tableMatrix, String... title) { + if (tableMatrix == null) { + log.warn("matrix is null, nothing to view") + return + } + viewTable(tableMatrix.columnNames(), tableMatrix.rowList(), tableMatrix.typeNames(), + matrixTitle(tableMatrix, title)) + } + + /** + * The title for a matrix view: an explicit non-blank title wins, then the matrix's + * own name, then a generic label. Matrix.builder() leaves matrixName null unless it + * is set, and the tab title is built by concatenation, so an unnamed matrix would + * otherwise be shown as "null (3 rows)". + */ + @PackageScope + static String matrixTitle(Matrix matrix, String... title) { + if (title.length > 0 && title[0] != null && !title[0].trim().isEmpty()) { + return title[0] + } + String name = matrix?.matrixName + return name == null || name.trim().isEmpty() ? 'Table' : name + } +``` + +- [x] **Step 4: Make the OS check locale-independent** + +In the same file, replace: + +```groovy + static final KeyCodeCombination KEY_CODE_COPY = + System.getProperty("os.name").toLowerCase().contains("mac") ? +``` + +with: + +```groovy + // Locale.ROOT: the Turkish locale lowercases 'I' to a dotless 'ı', which would + // break the check on a Turkish-locale JVM. AbstractInOut.isWindows does the same. + static final KeyCodeCombination KEY_CODE_COPY = + System.getProperty("os.name", "").toLowerCase(Locale.ROOT).contains("mac") ? +``` + +- [x] **Step 5: Guard the console and Swing implementations the same way** + +In `gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy`, replace: + +```groovy + @Override + void view(Matrix tableMatrix, String... title) { + println tableMatrix.content() + } +``` + +with: + +```groovy + @Override + void view(Matrix tableMatrix, String... title) { + if (tableMatrix == null) { + println("Nothing to view: the matrix is null") + return + } + println tableMatrix.content() + } +``` + +In `gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy`, replace: + +```groovy + @Override + void view(Matrix tableMatrix, String... title) { + Vector rows = new Vector(tableMatrix.rowCount()) +``` + +with: + +```groovy + @Override + void view(Matrix tableMatrix, String... title) { + if (tableMatrix == null) { + log.warn("Cannot view table: the matrix is null") + return + } + Vector rows = new Vector(tableMatrix.rowCount()) +``` + +Note for the reviewer: the Swing guard has no unit test. `gi-swing` runs its tests headless and the `InOut` constructor throws when headless, so no instance method of that class is reachable from a test; the guard is a two-line mirror of the tested `Viewer` one. + +- [x] **Step 6: Run the tests to verify they pass** + +Run: `./gradlew :gi-fx:test :gi-console:test :gi-swing:test` +Expected: `BUILD SUCCESSFUL`. + +- [x] **Step 7: Document it** + +In `docs/API-Guide.md`, in the *gi-fx* bullet list, append: + +```markdown +- `view((Matrix) null)` is logged and ignored rather than thrown, and a matrix with no name is titled `Table` instead of `null`. +``` + +In the *gi-console* bullet list, append: + +```markdown +- `view((Matrix) null)` prints a message on stdout rather than throwing. +``` + +- [x] **Step 8: Commit** + +```bash +git add gi-fx/src/main/groovy/se/alipsa/gi/fx/Viewer.groovy gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy gi-console/src/main/groovy/se/alipsa/gi/console/InOut.groovy gi-console/src/test/groovy/se/alipsa/gi/console/ConsolePromptTest.groovy gi-swing/src/main/groovy/se/alipsa/gi/swing/InOut.groovy docs/API-Guide.md +git commit -m "guard table views against a null matrix and title unnamed ones + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 8: Remove the rowsForViewer identity helper + +**Closes:** Finding 9 + +`rowsForViewer(matrix)` returns its argument unchanged, and its only test asserts `assertSame(rows, InOut.rowsForViewer(rows))` — it documents an intention (`Viewer.viewTable(List)` tolerates ragged and null rows) that is already pinned by `TableDataTest`'s `widestRow` and `formatRow` tests. It is indirection with no behaviour. + +**Files:** +- Modify: `gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy:443-455` (4-space indentation) +- Modify: `gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy` + +**Interfaces:** +- Consumes: `Viewer.viewTable(List> rows, String... title)`, which already handles null, empty and ragged input. +- Produces: `InOut.rowsForViewer` no longer exists. No other caller referenced it (`grep -rn rowsForViewer gi-fx/src` returns only the two sites below). + +- [x] **Step 1: Confirm there are exactly two references** + +Run: `grep -rn 'rowsForViewer' gi-fx/src` +Expected: one hit in `InOut.groovy` (the declaration plus its one call) and one test in `InOutHelperTest.groovy`. + +- [x] **Step 2: Inline the call** + +In `gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy`, replace: + +```groovy + @Override + void view(List> matrix, String... title) { + List> rows = rowsForViewer(matrix) + Platform.runLater { + Viewer.viewTable(rows, title) + } + } + + /** Preserves ragged and null rows for the JavaFX table renderer. */ + @PackageScope + static List> rowsForViewer(List> matrix) { + return matrix + } +``` + +with: + +```groovy + /** + * Ragged and null rows are passed through untouched: Viewer.viewTable sizes the + * table from the widest row and renders a null row as an empty one. + */ + @Override + void view(List> matrix, String... title) { + Platform.runLater { + Viewer.viewTable(matrix, title) + } + } +``` + +- [x] **Step 3: Delete the test that asserted identity** + +In `gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy`, delete: + +```groovy + @Test + void viewingAListPreservesRaggedAndNullRowsForTheViewer() { + List> rows = [['first'], null, ['second', 'third']] + + assertSame(rows, InOut.rowsForViewer(rows)) + } +``` + +and delete the now-unused import `import static org.junit.jupiter.api.Assertions.assertSame`. + +- [x] **Step 4: Add the behaviour test where it belongs** + +The property the deleted test gestured at — ragged and null rows survive into the rendered table — belongs on `Viewer`. Add to `gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy`: + +```groovy + @Test + void aRaggedGridKeepsEveryColumnAndRendersNullRowsAsEmpty() { + List> rows = [['first'], null, ['second', 'third']] + + assertEquals(2, Viewer.widestRow(rows)) + assertEquals(['first'], Viewer.formatRow(rows[0], NumberFormat.getInstance())) + assertEquals([], Viewer.formatRow(rows[1], NumberFormat.getInstance())) + assertEquals('', TableData.cellAt(Viewer.formatRow(rows[0], NumberFormat.getInstance()), 1)) + } +``` + +- [x] **Step 5: Run the tests** + +Run: `./gradlew :gi-fx:test` +Expected: `BUILD SUCCESSFUL`, with `InOutHelperTest` and `TableDataTest` both green. + +- [x] **Step 6: Verify the whole build** + +Run: `./gradlew check` +Expected: `BUILD SUCCESSFUL` (SpotBugs and Spotless included, after Task 2). + +- [x] **Step 7: Commit** + +```bash +git add gi-fx/src/main/groovy/se/alipsa/gi/fx/InOut.groovy gi-fx/src/test/groovy/se/alipsa/gi/fx/InOutHelperTest.groovy gi-fx/src/test/groovy/se/alipsa/gi/fx/TableDataTest.groovy +git commit -m "drop the rowsForViewer identity helper + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 9: Cover the Windows shell command path + +**Closes:** Finding 14 + +`shellCommand` chooses between `cmd.exe /d /s /c` and `/bin/sh -c` by reading `os.name`, and nothing tests the Windows branch: CI runs on `ubuntu-latest` only, and the tests that would exercise it call `assumeFalse(AbstractInOut.isWindows())`. Making the platform a parameter puts both branches under test from either OS, and a Windows smoke job catches what a unit test cannot. + +**Files:** +- Modify: `gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy:282-289` +- Test: `gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy` +- Modify: `.github/workflows/ci.yml` (append a job) + +**Interfaces:** +- Consumes: `@PackageScope static boolean isWindows()`, unchanged. +- Produces two `@PackageScope` overloads on `se.alipsa.gi.AbstractInOut`: + - `static List shellCommand(String command, boolean windows)` — reads the Windows interpreter from `ComSpec`. + - `static List shellCommand(String command, boolean windows, String commandInterpreter)` — takes the interpreter as an argument, so the `ComSpec`-unset fallback is testable without mutating the environment. + The existing `private static List shellCommand(String command)` stays and delegates to the two-argument form. + +- [x] **Step 1: Write the failing tests** + +In `gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy`, add next to the other shell tests: + +```groovy + @Test + void theWindowsShellUsesTheComSpecInterpreter() { + assertEquals(['C:\\Windows\\System32\\cmd.exe', '/d', '/s', '/c', 'dir'], + AbstractInOut.shellCommand('dir', true, 'C:\\Windows\\System32\\cmd.exe')) + } + + @Test + void theWindowsShellFallsBackToCmdWhenComSpecIsUnset() { + // A stripped environment has no ComSpec; without the fallback ProcessBuilder + // would be handed a null program name. The interpreter is passed in rather than + // read from the environment, so this holds on every platform -- an assertion + // that recomputed it the way the implementation does could never fail. + assertEquals(['cmd.exe', '/d', '/s', '/c', 'dir'], AbstractInOut.shellCommand('dir', true, null)) + assertEquals(['cmd.exe', '/d', '/s', '/c', 'dir'], AbstractInOut.shellCommand('dir', true, '')) + } + + @Test + void theUnixShellRunsThroughBinSh() { + assertEquals(['/bin/sh', '-c', 'ls -a'], AbstractInOut.shellCommand('ls -a', false)) + assertEquals(['/bin/sh', '-c', 'ls -a'], AbstractInOut.shellCommand('ls -a', false, 'ignored')) + } +``` + +- [x] **Step 2: Run them to make sure they fail** + +Run: `./gradlew :gi-common:test --tests '*AbstractInOutTest.the*Shell*'` +Expected: FAIL — compilation error, `Cannot find matching method se.alipsa.gi.AbstractInOut#shellCommand(java.lang.String, boolean)`; neither overload exists yet. + +- [x] **Step 3: Make the platform a parameter** + +In `gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy`, replace: + +```groovy + private static List shellCommand(String command) { + if (isWindows()) { + String commandInterpreter = System.getenv('ComSpec') + commandInterpreter = commandInterpreter ?: 'cmd.exe' + return [commandInterpreter, '/d', '/s', '/c', command] + } + return ['/bin/sh', '-c', command] + } +``` + +with: + +```groovy + private static List shellCommand(String command) { + return shellCommand(command, isWindows()) + } + + /** + * The argv used to run a command through the platform shell, taking the Windows + * command interpreter from the environment. + */ + @PackageScope + static List shellCommand(String command, boolean windows) { + return shellCommand(command, windows, System.getenv('ComSpec')) + } + + /** + * The argv used to run a command through the platform shell. Both the platform and + * the Windows command interpreter are parameters so every branch is testable from + * any operating system: CI runs the Linux branch, and a test that recomputed the + * interpreter from the environment the way this method does could never fail. + * ComSpec names the interpreter on Windows and is absent from a stripped + * environment, hence the cmd.exe fallback. + */ + @PackageScope + static List shellCommand(String command, boolean windows, String commandInterpreter) { + if (windows) { + return [commandInterpreter ?: 'cmd.exe', '/d', '/s', '/c', command] + } + return ['/bin/sh', '-c', command] + } +``` + +- [x] **Step 4: Run the tests to verify they pass** + +Run: `./gradlew :gi-common:test --tests '*AbstractInOutTest'` +Expected: PASS, the whole class green. + +- [x] **Step 5: Mark the tests that cannot run on Windows** + +Three tests use `printf`, which is not a `cmd.exe` builtin. Add to the imports of `AbstractInOutTest.groovy`: + +```groovy +import org.junit.jupiter.api.condition.DisabledOnOs +import org.junit.jupiter.api.condition.OS +``` + +and annotate these three tests with `@DisabledOnOs(OS.WINDOWS)` (placed directly under their `@Test`): + +- `shPrintsStandardOutputToSystemOut` +- `shQuietOverloadSuppressesLiveOutput` +- `shellSuccessReportsExitCodeAndResultStringIsDiagnostic` + +For example: + +```groovy + @Test + @DisabledOnOs(OS.WINDOWS) + void shPrintsStandardOutputToSystemOut() { +``` + +The remaining shell tests either already call `assumeFalse(AbstractInOut.isWindows())` or use `echo`, which both shells provide. + +- [x] **Step 6: Verify the suite is still green on Linux** + +Run: `./gradlew :gi-common:test` +Expected: `BUILD SUCCESSFUL`, with no tests skipped on this machine (the `@DisabledOnOs` annotations are inert on Linux). + +- [x] **Step 7: Add the Windows smoke job** + +Append to `.github/workflows/ci.yml`: + +```yaml + windows-smoke: + runs-on: windows-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up JDK 21 with JavaFX + uses: actions/setup-java@v4 + with: + distribution: 'liberica' + java-version: '21' + java-package: 'jdk+fx' + + - name: Setup Gradle + uses: gradle/actions/setup-gradle@v4 + + - name: Run the platform-sensitive tests on Windows + run: .\gradlew.bat :gi-common:test :gi-console:test + + - name: Upload test results + uses: actions/upload-artifact@v4 + if: always() + with: + name: windows-test-results + path: '**/build/reports/tests/' + retention-days: 7 +``` + +`.\gradlew.bat` is deliberate: the default shell on `windows-latest` is PowerShell, which would try to run the POSIX `gradlew` script. + +- [x] **Step 8: Verify the workflow parses** + +Run: +```bash +python3 -c "import yaml; d=yaml.safe_load(open('.github/workflows/ci.yml')); print(sorted(d['jobs']))" +``` +Expected: `['build', 'dependency-check', 'windows-smoke']`. + +Note for the reviewer: the Windows job itself can only be verified by pushing the branch. Its content is a copy of the `build` job's setup with a narrowed task list. + +- [x] **Step 9: Commit** + +```bash +git add gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy .github/workflows/ci.yml +git commit -m "test both shell command platforms and add a windows ci job + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 10: Make urlExists failures diagnosable + +**Closes:** Finding 13 + +`urlExists` returns a bare `false` for two different kinds of failure and says nothing about either: a thrown `RuntimeException`/`IOException` (a wrong proxy, an expired certificate, an unknown host) and a response that simply is not 2xx (a genuine 404 or 403, which never throws at all). Both get a debug line. The remaining `false` paths — a non-HTTP scheme, a redirect without a `Location`, and the redirect limit — stay silent; they are rare and self-explanatory from the URL, and widening the change further is out of scope for this task. + +`AbstractInOut` has no logger yet; the other modules all use `se.alipsa.matrix.core.util.Logger`, which comes in through `gi-common`'s `api libs.matrix.core`. + +**The message is built by concatenation, not by `{}` placeholders.** That logger only substitutes `{}` when SLF4J is on the classpath, and nothing in this project brings a binding — verified: `Logger.isSlf4jAvailable()` is `false` on the `gi-console` fat-jar classpath, and `log.warn("MARKER {}", 'x')` prints the literal `MARKER {}` through the `java.util.logging` fallback. Existing `{}` call sites elsewhere in the repo have the same wart; fixing those is not in scope here, but new lines must not add to it. + +**Files:** +- Modify: `gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy:24-31` (imports and fields), `:79` (the non-2xx return) and `:86-88` (the catch block) +- Test: `gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy` + +**Interfaces:** +- Consumes: `se.alipsa.matrix.core.util.Logger.getLogger(Class)`, with instance methods `debug(String)` and `isDebugEnabled()`. +- Produces: `private static final Logger log` on `AbstractInOut`. Behaviour is unchanged: `urlExists` still returns `false` for every failure. + +- [x] **Step 1: Pin the unchanged behaviour first** + +`AbstractInOutTest` already asserts that a malformed URL, an unknown host and a closed port all return `false`. Those three are named `testUrlExistsWithMalformedUrl`, `testUrlExistsWithUnreachableHost` and `testUrlExistsWithInvalidPort`, while the redirect and HEAD-fallback tests are named `urlExists*` — a single `--tests '*AbstractInOutTest.urlExists*'` pattern matches only the latter group and silently skips the baseline this step is about, so pass both patterns. + +Run: +```bash +./gradlew :gi-common:test --tests '*AbstractInOutTest.testUrlExists*' --tests '*AbstractInOutTest.urlExists*' --rerun-tasks +``` +Expected: PASS — 18 test cases across 11 methods (`urlExistsAppliesTheHeadFallbackAllowList` is parameterised into 8 invocations). + +- [x] **Step 2: Add the logger** + +In `gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy`, add to the imports: + +```groovy +import se.alipsa.matrix.core.util.Logger +``` + +and add as the first member of the class, above `private static final int MAX_REDIRECTS = 5`: + +```groovy + private static final Logger log = Logger.getLogger(AbstractInOut.class) +``` + +- [x] **Step 3: Log the swallowed exception** + +Replace: + +```groovy + } catch (RuntimeException | IOException ignored) { + return false + } +``` + +with: + +```groovy + } catch (RuntimeException | IOException e) { + // The contract is a boolean, so the cause is logged rather than thrown: + // a wrong proxy, an expired certificate and an unknown host are otherwise + // indistinguishable to the caller. Concatenated, not "{}": the logger only + // substitutes placeholders when SLF4J is present, and nothing here supplies it. + if (log.isDebugEnabled()) { + log.debug("urlExists(" + urlString + ") failed: " + e) + } + return false + } +``` + +- [x] **Step 4: Log a response that is not 2xx** + +A 404 or 403 never reaches the catch block — it returns `false` through the normal path — so without this the most common failure of all stays invisible. Replace: + +```groovy + return responseCode >= 200 && responseCode < 300 +``` + +with: + +```groovy + boolean reachable = responseCode >= 200 && responseCode < 300 + if (!reachable && log.isDebugEnabled()) { + log.debug("urlExists(" + url + ") got HTTP " + responseCode) + } + return reachable +``` + +`url` rather than `urlString`: after a redirect it is the address that actually answered. + +- [x] **Step 5: Add a test that the contract is unchanged for a failing connection** + +In `AbstractInOutTest.groovy`, add: + +```groovy + @Test + void urlExistsStillReportsFalseWhenTheConnectionFails() { + // The catch block now logs; the boolean contract must not change. + assertFalse(inOut.urlExists("http://127.0.0.1:1/health", 500)) + } +``` + +- [x] **Step 6: Run the tests** + +Run: `./gradlew :gi-common:test --tests '*AbstractInOutTest'` +Expected: PASS. + +- [x] **Step 7: See both diagnostics once, by hand** + +`Logger.setLevel(LogLevel.DEBUG)` is not enough on its own. With no SLF4J binding the +logger falls through to `java.util.logging`, whose default console handler drops +anything below `INFO`, so a debug line is filtered out before it is printed. The +handler has to be opened up too — verified during planning: without the properties +file below, nothing appears at all. + +Run: +```bash +./gradlew -q :gi-console:shadowJar +cat > /tmp/jul-fine.properties <<'PROPS' +handlers=java.util.logging.ConsoleHandler +.level=FINE +java.util.logging.ConsoleHandler.level=FINE +PROPS +cat > /tmp/urlexists-log-check.groovy <<'CHECK' +import com.sun.net.httpserver.HttpServer +import se.alipsa.matrix.core.util.Logger +import se.alipsa.matrix.core.util.LogLevel +Logger.setLevel(LogLevel.DEBUG) +def io = new se.alipsa.gi.console.InOut() + +println "unknown host -> " + io.urlExists('http://this-host-does-not-exist-12345.invalid/', 2000) + +HttpServer server = HttpServer.create(new InetSocketAddress('127.0.0.1', 0), 0) +server.createContext('/missing') { exchange -> exchange.sendResponseHeaders(404, -1); exchange.close() } +server.start() +try { + println "404 -> " + io.urlExists("http://127.0.0.1:${server.address.port}/missing", 2000) +} finally { + server.stop(0) +} +CHECK +java -Djava.util.logging.config.file=/tmp/jul-fine.properties \ + -cp gi-console/build/libs/gi-console-0.4.1-SNAPSHOT-fatjar.jar:$(ls ~/.sdkman/candidates/groovy/current/lib/groovy-5*.jar | head -1) \ + groovy.ui.GroovyMain /tmp/urlexists-log-check.groovy +``` +Expected: both calls print `false`, and each is preceded by a `FINE:` line — one naming `UnknownHostException` with the URL spelled out, one reading `urlExists(http://127.0.0.1:/missing) got HTTP 404`. Neither line may contain a literal `{}`. + +- [x] **Step 8: Commit** + +```bash +git add gi-common/src/main/groovy/se/alipsa/gi/AbstractInOut.groovy gi-common/src/test/groovy/se/alipsa/gi/AbstractInOutTest.groovy +git commit -m "log why urlExists returns false + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 11: Deduplicate licence and notice files in the fat JARs + +**Closes:** Finding 10 + +`duplicatesStrategy = INCLUDE` is required for `mergeServiceFiles()` to work, but it also writes one copy of every non-merged duplicate resource into the archive: the current `gi-console` fat JAR contains 8 duplicated paths (`LICENSE`, `NOTICE`, `META-INF/LICENSE`, `META-INF/LICENSE.txt`, `META-INF/NOTICE`, `META-INF/NOTICE.txt`, `META-INF/DEPENDENCIES`, `META-INF/buildinfo.xml`). The licence texts of bundled dependencies must stay in the artifact, so they are merged rather than dropped. The exact configuration below was applied experimentally during planning: it takes the duplicate list to empty while leaving the merged service and Groovy extension-module files intact. + +**Files:** +- Modify: `buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle` +- Modify: `docs/DEPENDENCIES.md` (the *Fat JARs* section) + +**Interfaces:** +- Consumes: `rootProject.file('LICENSE')` — the repository's own MIT licence, which becomes the header of the merged licence file. +- Produces: no API change. `shadowJar` still publishes `--fatjar.jar`. + +- [x] **Step 1: Record the duplicates as the failing check** + +Run: +```bash +./gradlew -q :gi-console:shadowJar +unzip -l gi-console/build/libs/gi-console-0.4.1-SNAPSHOT-fatjar.jar | awk 'NF>=4{print $4}' | sort | uniq -d +``` +Expected (this is the bug): eight paths are listed. + +- [x] **Step 2: Merge the licence, notice and dependency metadata** + +In `buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle`, append inside the existing `tasks.named('shadowJar', ...)` block, after the `exclude 'module-info.class', ...` line: + +```groovy + + // INCLUDE keeps every duplicate entry, so without these the archive holds one + // LICENSE, NOTICE and DEPENDENCIES file per source jar. The bundled + // dependencies' licence texts have to stay in the artifact, so they are merged + // into a single entry each rather than dropped. + transform(com.github.jengelman.gradle.plugins.shadow.transformers.MergeLicenseResourceTransformer) { + artifactLicense = rootProject.file('LICENSE') + artifactLicenseSpdxId = 'MIT' + } + transform(com.github.jengelman.gradle.plugins.shadow.transformers.ApacheNoticeResourceTransformer) { + projectName = 'GuiInteraction' + } + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'META-INF/DEPENDENCIES' + } + // ApacheNoticeResourceTransformer only claims META-INF/NOTICE*; several jars + // also ship a NOTICE at the archive root. + transform(com.github.jengelman.gradle.plugins.shadow.transformers.AppendingTransformer) { + resource = 'NOTICE' + } + + // Reproducible-build metadata describing a source jar, meaningless once that + // jar is repackaged, and duplicated once per dependency. + exclude 'META-INF/buildinfo.xml' +``` + +- [x] **Step 3: Verify the duplicates are gone and nothing was lost** + +Run: +```bash +./gradlew -q :gi-console:shadowJar :gi-swing:shadowJar :gi-fx:shadowJar +for m in gi-console gi-swing gi-fx; do + J=$m/build/libs/$m-0.4.1-SNAPSHOT-fatjar.jar + echo "== $m duplicates:"; unzip -l "$J" | awk 'NF>=4{print $4}' | sort | uniq -d + echo "== $m license header:"; unzip -p "$J" META-INF/LICENSE | head -2 +done +unzip -p gi-console/build/libs/gi-console-0.4.1-SNAPSHOT-fatjar.jar META-INF/groovy/org.codehaus.groovy.runtime.ExtensionModule | grep extensionClasses +``` +Expected: no duplicate paths for any module; `META-INF/LICENSE` starts with `SPDX-License-Identifier: MIT` followed by `MIT License`; the extension-module line still lists all four merged extension classes: + +``` +extensionClasses=se.alipsa.matrix.core.ext.RowExtension,se.alipsa.matrix.core.ext.ColumnExtension,se.alipsa.matrix.core.ext.PipeExtension,se.alipsa.matrix.ext.NumberExtension +``` + +- [x] **Step 4: Verify a fat JAR still runs** + +Run: +```bash +cat > /tmp/fatjar-smoke.groovy <<'CHECK' +def io = new se.alipsa.gi.console.InOut() +io.view([[1, 'Foo'], [2, 'Bar']], 'smoke') +println io.getContentType('src/test/resources/areachart2.png') +CHECK +java -cp gi-console/build/libs/gi-console-0.4.1-SNAPSHOT-fatjar.jar:$(ls ~/.sdkman/candidates/groovy/current/lib/groovy-5*.jar | head -1) groovy.ui.GroovyMain /tmp/fatjar-smoke.groovy +``` +Expected: the table prints and the last line is `image/png` — Tika's service registrations survived the transform. + +- [x] **Step 5: Document it** + +In `docs/DEPENDENCIES.md`, append to the *Fat JARs* section: + +```markdown +`duplicatesStrategy = INCLUDE` is what makes `mergeServiceFiles()` possible, but it +also copies every non-merged duplicate resource once per source jar. The +`MergeLicenseResourceTransformer`, `ApacheNoticeResourceTransformer` and two +`AppendingTransformer`s merge the licence, notice and `META-INF/DEPENDENCIES` +files into one entry each, so the bundled dependencies' licence texts are kept +without duplicating them; `META-INF/buildinfo.xml`, which describes a source jar +and means nothing after repackaging, is excluded. A fat JAR should contain no +duplicate paths: + +```bash +unzip -l gi-console/build/libs/gi-console--fatjar.jar \ + | awk 'NF>=4{print $4}' | sort | uniq -d +``` +``` + +- [x] **Step 6: Commit** + +```bash +git add buildSrc/src/main/groovy/se.alipsa.gi.fatjar-conventions.gradle docs/DEPENDENCIES.md +git commit -m "merge duplicate license and notice files in the fat jars + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 12: Promote the Unreleased section instead of writing above it + +**Closes:** Findings 3, 12 (the `echo -e` half) + +`generate_release_notes` inserts the new section after line 2 of `release.md`. Line 3 is `## Unreleased`, which holds the hand-written notes for the version being released. After a release the file therefore reads `## - ` — containing nothing but commit subjects — immediately followed by `## Unreleased`, still holding the notes that were just released. The function is also unreachable on the path where `build.gradle` already holds a non-SNAPSHOT version, and it pipes commit subjects through `echo -e`, which eats backslash escapes. + +Extracting the logic into a sourceable library is what makes it testable: `release.sh` runs its whole flow at the top level and cannot be sourced. + +**Files:** +- Create: `release-lib.sh` +- Modify: `release.sh:23-29` (after `set -e`, to source the library), `release.sh:221-262` (`generate_release_notes`), `release.sh:300-314` (`commit_release_version`), `release.sh:349-375` (the call sites) +- Modify: `docs/PUBLISHING.md` + +**Interfaces:** +- Consumes: nothing. +- Originally planned four shell functions in `release-lib.sh`; the shipped library also includes `has_curated_unreleased_notes`, `require_clean_release_file`, and `commit_changed_release_files`: + - `release_section_exists [file]` — true when the changelog already holds a `## - ` heading. + - `promote_unreleased_section [file]` — returns **0** and rewrites the file when a `## Unreleased` heading was promoted, **1** when there is no such heading (the caller falls back to the commit log), and **2** when the file already has a section for this version, in which case the file is left untouched. + - `insert_release_section [file]` — inserts a `## - ` section built from commit subjects before the first `## ` heading, or appends it if the file has none; returns **2** and changes nothing when the version already has a section. Both update functions also return **3** on I/O failure. + - `validate_version ` — wired in by Task 13. + + The **2** return codes are what make a release retry safe. Promotion opens a fresh + empty `## Unreleased`; re-running the same version (the documented `--skip` + workflow after a partial publish) would otherwise promote that empty section into a + second heading for a version that already has one. + +- [x] **Step 1: Write the failing check against a fixture** + +Run: +```bash +mkdir -p /tmp/relnotes && cd /tmp/relnotes +printf '# Gui Interaction Release Notes\n\n## Unreleased\n- Fixed: something important.\n\n## 0.4.0 - 2026-09-01\n- Older entry.\n' > release.md +cd /path/to/GuiInteraction +bash -c 'source ./release-lib.sh' 2>&1 | head -2 +``` +Expected (this is the bug): `bash: ./release-lib.sh: No such file or directory` — there is nothing to test yet, and the current in-script logic would insert a new heading *above* `## Unreleased`. + +- [x] **Step 2: Write the library** + +Create `release-lib.sh`: + +```bash +#!/usr/bin/env bash +# +# Changelog and version helpers for release.sh. +# +# This file is meant to be sourced, not executed, so the functions can be +# exercised against a fixture without running a release: +# +# source ./release-lib.sh +# promote_unreleased_section 0.4.1 2026-09-26 /tmp/release.md + +# True when the changelog already has a section for this version. Both writers +# below check it: re-running a release for the same version after a partial +# publish is an explicit workflow (release.sh --skip), and it must not add a +# second heading for that version. +release_section_exists() { + local version=$1 + local file=${2:-release.md} + [ -f "$file" ] && grep -qE "^## +${version//./[.]} - " "$file" +} + +# Turn the curated "## Unreleased" section into a released one and open a fresh +# empty "## Unreleased" above it. The hand-written notes stay where they are, +# under the version they describe. +# +# Returns 0 when a section was promoted, 1 when the file has no Unreleased +# heading (the caller then falls back to insert_release_section), and 2 when the +# file already has a section for this version, so there is nothing to do. +promote_unreleased_section() { + local version=$1 + local date=$2 + local file=${3:-release.md} + local tmp + + if [ ! -f "$file" ]; then + return 1 + fi + if release_section_exists "$version" "$file"; then + return 2 + fi + if ! grep -qE '^## +Unreleased[[:space:]]*$' "$file"; then + return 1 + fi + + tmp=$(mktemp) + awk -v heading="## ${version} - ${date}" ' + !promoted && /^## +Unreleased[[:space:]]*$/ { + print "## Unreleased" + print "" + print heading + promoted = 1 + next + } + { print } + ' "$file" > "$tmp" + mv "$tmp" "$file" +} + +# Insert a "## - " section built from commit subjects before the +# first existing "## " heading, or append it when the file has none. printf is +# used rather than echo -e so a commit subject containing a backslash survives. +# +# Returns 0 when a section was inserted and 2 when the file already has one for +# this version. +insert_release_section() { + local version=$1 + local date=$2 + local commits=$3 + local file=${4:-release.md} + local section tmp line + + if release_section_exists "$version" "$file"; then + return 2 + fi + + section=$(mktemp) + { + printf '## %s - %s\n\n' "$version" "$date" + if [ -n "$commits" ]; then + printf '### Changes\n\n' + while IFS= read -r line; do + if [ -n "$line" ]; then + printf -- '- %s\n' "${line#* }" + fi + done <<< "$commits" + printf '\n' + fi + } > "$section" + + if [ ! -f "$file" ]; then + printf '# Gui Interaction Release Notes\n\n' > "$file" + fi + + tmp=$(mktemp) + awk -v section="$section" ' + !inserted && /^## / { + while ((getline line < section) > 0) print line + close(section) + inserted = 1 + } + { print } + END { + if (!inserted) { + while ((getline line < section) > 0) print line + close(section) + } + } + ' "$file" > "$tmp" + mv "$tmp" "$file" + rm -f "$section" +} + +# Reject a version the release machinery cannot handle before anything is mutated. +# Two shapes are dangerous, for opposite reasons. A two-component version fails +# quietly: an empty field is 0 inside $(( )), so "0.4" patch-bumps to "0.4.1" with +# exit status 0, and the wrong version then flows into the commit, the tag and the +# three-part regexes used for the README. A leading zero fails loudly: 08 and 09 are +# invalid octal to bash, which aborts release.sh mid-flight under `set -e`. Semantic +# versioning forbids leading zeros anyway. +validate_version() { + local version=${1%-SNAPSHOT} + if [[ ! "$version" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then + echo "Unsupported version '${1}'. Expected MAJOR.MINOR.PATCH or MAJOR.MINOR.PATCH-SNAPSHOT, with no leading zeros." >&2 + return 1 + fi + return 0 +} +``` + +`validate_version` is written here but not wired in until Task 13. + +- [x] **Step 3: Verify promotion against the fixture** + +Run: +```bash +cd /tmp/relnotes +printf '# Gui Interaction Release Notes\n\n## Unreleased\n- Fixed: something important.\n\n## 0.4.0 - 2026-09-01\n- Older entry.\n' > release.md +source /path/to/GuiInteraction/release-lib.sh +promote_unreleased_section 0.4.1 2026-09-26 release.md && echo "promoted" +cat release.md +``` +Expected: + +``` +promoted +# Gui Interaction Release Notes + +## Unreleased + +## 0.4.1 - 2026-09-26 +- Fixed: something important. + +## 0.4.0 - 2026-09-01 +- Older entry. +``` + +- [x] **Step 4: Verify the fallback when there is no Unreleased section** + +Run: +```bash +cd /tmp/relnotes +printf '# Gui Interaction Release Notes\n\n## 0.4.0 - 2026-09-01\n- Older entry.\n' > release.md +source /path/to/GuiInteraction/release-lib.sh +promote_unreleased_section 0.4.1 2026-09-26 release.md || echo "nothing to promote" +insert_release_section 0.4.1 2026-09-26 "$(printf 'abc1234 fix a thing\ndef5678 add \\n handling')" release.md +cat release.md +``` +Expected: `nothing to promote`, then a file whose new section sits **above** the 0.4.0 one and whose second bullet reads `- add \n handling` with the backslash intact: + +``` +# Gui Interaction Release Notes + +## 0.4.1 - 2026-09-26 + +### Changes + +- fix a thing +- add \n handling + +## 0.4.0 - 2026-09-01 +- Older entry. +``` + +- [x] **Step 5: Verify a retry does not duplicate the section** + +`release.sh --skip` exists to resume a partial release with the *same* version, so +the changelog writers have to be idempotent. Run: + +```bash +cd /tmp/relnotes +printf '# Gui Interaction Release Notes\n\n## Unreleased\n- Fixed: something important.\n\n## 0.4.0 - 2026-09-01\n- Older entry.\n' > release.md +source /path/to/GuiInteraction/release-lib.sh +rc=0; promote_unreleased_section 0.4.1 2026-09-26 release.md || rc=$?; echo "first run rc=$rc" +rc=0; promote_unreleased_section 0.4.1 2026-09-27 release.md || rc=$?; echo "retry rc=$rc" +rc=0; insert_release_section 0.4.1 2026-09-27 'abc1234 something' release.md || rc=$?; echo "fallback on retry rc=$rc" +echo "headings for 0.4.1: $(grep -c '^## 0\.4\.1 - ' release.md)" +cat release.md +cd /path/to/GuiInteraction +``` +Expected: + +``` +first run rc=0 +retry rc=2 +fallback on retry rc=2 +headings for 0.4.1: 1 +# Gui Interaction Release Notes + +## Unreleased + +## 0.4.1 - 2026-09-26 +- Fixed: something important. + +## 0.4.0 - 2026-09-01 +- Older entry. +``` + +The second promotion must not turn the fresh empty `## Unreleased` into a second +`## 0.4.1` heading, and the commit-log fallback must refuse the same way. + +- [x] **Step 6: Source the library from release.sh** + +In `release.sh`, immediately after `set -e`, add: + +```bash + +# Changelog and version helpers, kept in a separate file so they can be sourced +# and tested against a fixture without running a release. +SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=release-lib.sh +source "${SCRIPT_DIR}/release-lib.sh" +``` + +- [x] **Step 7: Rewrite generate_release_notes to use it** + +Replace the whole `generate_release_notes` function body (from `local date=$(date +%Y-%m-%d)` to the end of the function) with: + +```bash +generate_release_notes() { + local version=$1 + local date + date=$(date +%Y-%m-%d) + local release_notes_file="release.md" + local rc=0 + + if [ ! -f "$release_notes_file" ]; then + printf '# Gui Interaction Release Notes\n\n' > "$release_notes_file" + fi + + # The curated "## Unreleased" section is the release notes. Promote it rather + # than writing a second, commit-derived section above it and leaving the real + # notes behind under a heading that says they are unreleased. + # `|| rc=$?` rather than `if promote...`: the function has three outcomes, and a + # bare call would abort the script under `set -e`. + promote_unreleased_section "$version" "$date" "$release_notes_file" || rc=$? + case $rc in + 0) + echo -e "${GREEN}Promoted the Unreleased section of ${release_notes_file} to ${version}${NC}" + return + ;; + 2) + echo -e "${YELLOW}${release_notes_file} already has a section for ${version}; leaving it as it is.${NC}" + return + ;; + esac + + local last_tag commits + last_tag=$(git describe --tags --abbrev=0 2>/dev/null || echo "") + if [ -n "$last_tag" ]; then + commits=$(git log --oneline "${last_tag}..HEAD" 2>/dev/null || echo "") + else + commits=$(git log --oneline -20 2>/dev/null || echo "") + fi + + rc=0 + insert_release_section "$version" "$date" "$commits" "$release_notes_file" || rc=$? + if [ "$rc" -eq 0 ]; then + echo -e "${GREEN}Updated ${release_notes_file} from the commit log${NC}" + else + echo -e "${YELLOW}${release_notes_file} already has a section for ${version}; leaving it as it is.${NC}" + fi +} +``` + +- [x] **Step 8: Make the notes run on the already-released-version path too** + +The third branch of the version decision (`build.gradle` already holds a non-SNAPSHOT version) never calls `commit_release_version`, so no notes are written. In `release.sh`, replace: + +```bash +elif [ "$README_NEEDS_UPDATE" = true ]; then + if [ "$DRY_RUN" = true ]; then + echo -e "${YELLOW}[DRY RUN] Would update README.md to match version ${CURRENT_VERSION}${NC}" + else + read -p "Update README.md to version ${CURRENT_VERSION}? [Y/n]: " update_readme + if [[ ! "$update_readme" =~ ^[Nn]$ ]]; then + update_readme_version "$CURRENT_VERSION" + if ! git diff --quiet HEAD -- README.md; then + if ! git add README.md; then + echo -e "${RED}Error: Failed to add README.md to git. Please resolve the issue and try again.${NC}" >&2 + exit 1 + fi + if ! git commit -m "Update README version to ${CURRENT_VERSION}" -- README.md; then + echo -e "${RED}Error: Failed to commit README.md version change. Please resolve the issue and try again.${NC}" >&2 + exit 1 + fi + else + echo -e "${YELLOW}README.md already matches version ${CURRENT_VERSION}; no commit needed.${NC}" + fi + fi + fi +fi +``` + +with: + +```bash +else + if [ "$DRY_RUN" = true ]; then + echo -e "${YELLOW}[DRY RUN] Would update release.md for ${CURRENT_VERSION}${NC}" + if [ "$README_NEEDS_UPDATE" = true ]; then + echo -e "${YELLOW}[DRY RUN] Would update README.md to match version ${CURRENT_VERSION}${NC}" + fi + else + # build.gradle already holds the release version, so no version bump is + # committed here -- but the changelog still has to be written. + generate_release_notes "$CURRENT_VERSION" + if [ "$README_NEEDS_UPDATE" = true ]; then + read -p "Update README.md to version ${CURRENT_VERSION}? [Y/n]: " update_readme + if [[ ! "$update_readme" =~ ^[Nn]$ ]]; then + update_readme_version "$CURRENT_VERSION" + fi + fi + if ! git diff --quiet HEAD -- README.md release.md; then + if ! git add README.md release.md; then + echo -e "${RED}Error: Failed to add files to git. Please resolve the issue and try again.${NC}" >&2 + exit 1 + fi + if ! git commit -m "Update release notes for ${CURRENT_VERSION}" -- README.md release.md; then + echo -e "${RED}Error: Failed to commit the release notes. Please resolve the issue and try again.${NC}" >&2 + exit 1 + fi + else + echo -e "${YELLOW}README.md and release.md already match version ${CURRENT_VERSION}; no commit needed.${NC}" + fi + fi +fi +``` + +- [x] **Step 9: Verify the script still parses and the dry run is unchanged** + +Run: +```bash +bash -n release.sh && echo "release.sh parses" +bash -n release-lib.sh && echo "release-lib.sh parses" +./release.sh --dry-run 2>&1 | tail -20 +``` +Expected: both parse; the dry run reports the version it would release, the four `[DRY RUN] Would execute: ./gradlew ::clean ...` lines, and the GitHub release it would create. `git status --porcelain` must be empty afterwards — a dry run mutates nothing. + +- [x] **Step 10: Verify a real promotion against a copy of the real changelog** + +The repository's own `release.md` is the file this has to work on, so check it — but +against a copy, and with the *working tree's* library. Do not `git clone` here: +`release-lib.sh` is not committed until Step 12, so a clone would not contain it. + +Run: +```bash +rm -rf /tmp/relnotes-real && mkdir -p /tmp/relnotes-real +cp release.md /tmp/relnotes-real/ +REPO=$PWD +cd /tmp/relnotes-real +source "$REPO/release-lib.sh" +promote_unreleased_section 0.4.1 "$(date +%Y-%m-%d)" release.md && head -8 release.md +grep -c '^## Unreleased$' release.md +cd "$REPO" +git status --porcelain release.md +``` +Expected: the copied `release.md` gains an empty `## Unreleased` (`grep -c` prints `1`) with the real v0.4.1 notes now under `## 0.4.1 - `; the last command prints nothing, because the repository's own `release.md` was not touched. + +- [x] **Step 11: Document the changelog contract** + +In `docs/PUBLISHING.md`, add a section: + +```markdown +## Release notes + +`release.md` carries a `## Unreleased` heading at the top. Write each change under +that heading as it is merged. At release time `release.sh` promotes that section: +the heading becomes `## - ` and a fresh empty `## Unreleased` is +opened above it, so the notes that describe a release stay with that release. + +If the heading is missing, the script falls back to generating a section from the +commit subjects since the last tag. Both writers are idempotent: if the changelog +already holds a `## - ` heading they change nothing and report it, so +resuming a partial release with `--skip` and the same version does not add a second +section for it. + +The helpers live in `release-lib.sh` and are sourced, so they can be checked against +a fixture without running a release: + +```bash +source ./release-lib.sh +promote_unreleased_section 1.2.3 2026-01-31 /tmp/release.md # 0 promoted, 1 nothing to promote, 2 already recorded +``` +``` + +- [x] **Step 12: Commit** + +```bash +git add release.sh release-lib.sh docs/PUBLISHING.md +git commit -m "promote the unreleased changelog section at release time + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 13: Validate the version before the release mutates anything + +**Closes:** Finding 12 (the `bump_version` half) + +`bump_version` splits the version on `.` and does arithmetic on the fields. Two shapes go wrong, in opposite ways, both after the tag check and part-way into the release: + +- **`0.4` fails quietly.** An empty variable is `0` inside `$(( ))`, so a patch bump of `0.4` yields `0.4.1` and a minor bump yields `0.5.0` — exit status 0, no message. That wrong version then flows into the release commit, the tag and the published coordinates, and `check_readme_version` / `update_readme_version`, whose regexes are three-part, cannot match it. *(An earlier draft of this plan predicted `syntax error: operand expected` here. That was wrong; Step 1 below shows what bash actually does.)* +- **`0.08.1` fails loudly.** Bash reads a leading zero as octal: `08: value too great for base (error token is "08")`, exit 1. Under `release.sh`'s `set -e` that aborts the release part-way through. + +`validate_version` (added to `release-lib.sh` in Task 12) rejects both shapes up front, and `bump_version` is made base-10 explicit so the arithmetic is safe no matter who calls it. + +**Files:** +- Modify: `release.sh:155-183` (`bump_version`) and the main flow right after `CURRENT_VERSION=$(get_version)` +- Modify: `release-lib.sh` (only if Task 12's `validate_version` is missing) + +**Interfaces:** +- Consumes: `validate_version ` from `release-lib.sh` — returns 0 for `MAJOR.MINOR.PATCH[-SNAPSHOT]` with no leading zeros, else prints to stderr and returns 1. +- Produces: nothing new. + +- [x] **Step 1: Reproduce the failure** + +Run: +Run each input in its own shell: an arithmetic error aborts the rest of the command +list it occurs in, so several cases in one script mask each other's output. + +```bash +cd /tmp && cat > bumpcheck.sh <<'CHECK' +bump_version() { + local version=$1 type=$2 major minor patch + IFS='.' read -r major minor patch <<< "${version%-SNAPSHOT}" + case $type in + minor) minor=$((minor + 1)); patch=0 ;; + patch) patch=$((patch + 1)) ;; + esac + echo "${major}.${minor}.${patch}" +} +bump_version "$1" "$2" +CHECK +for pair in 0.4:patch 0.4:minor 0.08.1:minor 0.4.09:patch; do + v=${pair%%:*}; t=${pair##*:} + out=$(bash bumpcheck.sh "$v" "$t" 2>&1); rc=$? + printf '%-8s %-6s -> %-66s exit=%d\n' "$v" "$t" "$out" "$rc" +done +cd /path/to/GuiInteraction +``` + +The `pair` list is split with `${pair%%:*}` rather than `set -- $pair` so the loop +behaves the same under bash and zsh. + +Expected — two different bugs, and the two-component one is the quieter and worse of +the pair: + +``` +0.4 patch -> 0.4.1 exit=0 +0.4 minor -> 0.5.0 exit=0 +0.08.1 minor -> bumpcheck.sh: line 5: 08: value too great for base (error token is "08") exit=1 +0.4.09 patch -> bumpcheck.sh: line 6: 09: value too great for base (error token is "09") exit=1 +``` + +`0.4` does not raise `syntax error: operand expected` — inside `$(( ))` an empty +variable is `0`, so the bump succeeds and returns a version that looks plausible and +is not the one anybody asked for. That is what the guard is for as much as the octal +case. + +- [x] **Step 2: Verify the guard accepts and rejects the right shapes** + +Run: +```bash +source ./release-lib.sh +for v in 0.4.1 0.4.1-SNAPSHOT 10.20.30 0.0.0; do validate_version "$v" && echo "accepted $v"; done +for v in 0.4 v0.4.1 0.4.1.2 '' 0.4.x 0.08.1 0.4.09 01.2.3; do validate_version "$v" 2>/dev/null || echo "rejected '$v'"; done +``` +Expected: the first four print `accepted ...`; the last eight print `rejected '...'`. The three leading-zero cases matter as much as the short one — `0.08.1` passes a naive `[0-9]+` regex and then dies in bash arithmetic. + +If `validate_version` is not present in `release-lib.sh`, add it there first using the definition given in Task 12, Step 2. + +- [x] **Step 3: Wire it into the release flow** + +In `release.sh`, replace: + +```bash +CURRENT_VERSION=$(get_version) +echo -e "Current version: ${YELLOW}${CURRENT_VERSION}${NC}" +``` + +with: + +```bash +CURRENT_VERSION=$(get_version) +echo -e "Current version: ${YELLOW}${CURRENT_VERSION}${NC}" + +# Reject an unsupported version shape before anything is mutated: bump_version +# does arithmetic on the patch field, and a failure there would land after the +# tag check, part-way through the release. +if ! validate_version "$CURRENT_VERSION"; then + echo -e "${RED}Aborting: build.gradle version '${CURRENT_VERSION}' is not MAJOR.MINOR.PATCH[-SNAPSHOT].${NC}" >&2 + exit 1 +fi +``` + +- [x] **Step 4: Also guard the computed release version** + +In `release.sh`, replace: + +```bash +if [ -n "$BUMP_TYPE" ]; then + RELEASE_VERSION=$(bump_version "$CURRENT_VERSION" "$BUMP_TYPE") + echo -e "Bumping version to: ${GREEN}${RELEASE_VERSION}${NC}" +``` + +with: + +```bash +if [ -n "$BUMP_TYPE" ]; then + RELEASE_VERSION=$(bump_version "$CURRENT_VERSION" "$BUMP_TYPE") + if ! validate_version "$RELEASE_VERSION"; then + echo -e "${RED}Aborting: the bumped version '${RELEASE_VERSION}' is malformed.${NC}" >&2 + exit 1 + fi + echo -e "Bumping version to: ${GREEN}${RELEASE_VERSION}${NC}" +``` + +- [x] **Step 5: Make bump_version base-10 explicit** + +The guard now keeps leading zeros out, but `bump_version` should not depend on its +caller for that. In `release.sh`, replace the `case` block inside `bump_version`: + +```bash + case $type in + major) + major=$((major + 1)) + minor=0 + patch=0 + ;; + minor) + minor=$((minor + 1)) + patch=0 + ;; + patch) + patch=$((patch + 1)) + ;; +``` + +with: + +```bash + # 10# forces base 10: bash reads a bare 08 or 09 as octal and fails with + # "value too great for base". validate_version already rejects leading zeros, + # so this is the second line of defence, not the first. + case $type in + major) + major=$((10#$major + 1)) + minor=0 + patch=0 + ;; + minor) + minor=$((10#$minor + 1)) + patch=0 + ;; + patch) + patch=$((10#$patch + 1)) + ;; +``` + +Leave the `*)` branch and the trailing `echo "${major}.${minor}.${patch}"` as they are. + +- [x] **Step 6: Verify the coercion handles what broke in Step 1** + +Run: +```bash +cd /tmp && cat > bumpcheck10.sh <<'CHECK' +bump_version() { + local version=$1 type=$2 major minor patch + IFS='.' read -r major minor patch <<< "${version%-SNAPSHOT}" + case $type in + minor) minor=$((10#$minor + 1)); patch=0 ;; + patch) patch=$((10#$patch + 1)) ;; + esac + echo "${major}.${minor}.${patch}" +} +bump_version "0.08.1" minor +bump_version "0.4.09" patch +bump_version "0.4.1" patch +CHECK +bash bumpcheck10.sh; echo "exit=$?" +cd /path/to/GuiInteraction +``` +Expected — no errors, and the octal cases now compute: + +``` +0.9.0 +0.4.10 +0.4.2 +exit=0 +``` + +- [x] **Step 7: Verify the script parses and the dry run still works** + +Run: +```bash +bash -n release.sh && echo "release.sh parses" +./release.sh --dry-run 2>&1 | head -12 +./release.sh --bump minor --dry-run 2>&1 | grep -i bumping +git status --porcelain +``` +Expected: the script parses; the plain dry run reports `Stripping SNAPSHOT suffix: 0.4.1-SNAPSHOT -> 0.4.1`; the bump dry run reports `Bumping version to: 0.5.0`; `git status --porcelain` prints nothing. + +- [x] **Step 8: Verify the guard fires on a malformed version** + +Copy the working tree's scripts rather than cloning: the changes from Steps 3 and 4 +are not committed yet, so a clone would run the previous version of `release.sh`. +`release.sh` aborts during version validation, before it needs a git repository, so +the three files it reads up to that point are enough. + +Run: +```bash +rm -rf /tmp/badversion && mkdir -p /tmp/badversion +cp release.sh release-lib.sh build.gradle /tmp/badversion/ +REPO=$PWD +cd /tmp/badversion +sed -i "s/^version = '0.4.1-SNAPSHOT'/version = '0.4'/" build.gradle +./release.sh --dry-run; echo "exit=$?" +cd "$REPO" +``` +Expected: the run stops with `Aborting: build.gradle version '0.4' is not MAJOR.MINOR.PATCH[-SNAPSHOT].` and `exit=1`, before any file is touched. + +- [x] **Step 9: Document it** + +In `docs/PUBLISHING.md`, add to the release-notes section written in Task 12: + +```markdown +`release.sh` refuses to start unless `build.gradle` holds a +`MAJOR.MINOR.PATCH[-SNAPSHOT]` version, and re-checks the version it computes for +`--bump`. +``` + +- [x] **Step 10: Commit** + +```bash +git add release.sh release-lib.sh docs/PUBLISHING.md +git commit -m "validate the version shape before releasing + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +## Task 14: Roll up the changelog and verify the whole branch + +**Closes:** Finding 15 (remaining docs), and gates the branch + +Every preceding task carried its own documentation. This one records the release-facing summary, closes the TODO items these fixes resolve, and runs the full verification set one last time. + +**Files:** +- Modify: `release.md` (the `## Unreleased` section) +- Modify: `TODO.md` + +**Interfaces:** +- Consumes: the finished work of Tasks 1–13. +- Produces: nothing. + +- [x] **Step 1: Add the changelog entries** + +In `release.md`, under `## Unreleased`, append: + +```markdown +- Fixed (build): the Java 21 toolchain was never applied to any module — the guard in the root `subprojects` block ran before the modules applied the `java` plugin, so every artifact was compiled at the Gradle JVM's language level. Modules now pin Java 21 through `pluginManager.withPlugin('java')`. +- Fixed (build): `./gradlew check` and `./gradlew build` skipped Spotless and SpotBugs entirely, because the configuration cache is on by default. Both now run as part of the default `check`; the workarounds they were disabled for are obsolete on Gradle 9.6.1. +- Fixed (build): the OWASP scan ran `dependencyCheckAnalyze` against the dependency-free root project and CI's `dependency-check` job ran `dependencyUpdates` instead of a scan. Both now use `dependencyCheckAggregate`, which covers all four modules. +- Fixed (build): the JavaFX platform classifier assumed aarch64 on every macOS and produced an unresolvable `unknown` on any other platform. A `se.alipsa.gi.javafx-platform` convention plugin now resolves `linux`, `linux-aarch64`, `mac`, `mac-aarch64` or `win` from the JVM's `os.arch`, and fails with the platform and architecture named for anything else — including an Arm64 JVM on Windows, where OpenJFX publishes no Arm64 build and an Arm64 process cannot load the x64 one. Build with an x64 JDK there. +- Fixed: console `display(String)` threw `NullPointerException` for a null file name; it now behaves like `display((File) null)` and reports the problem on stdout, matching `gi-swing` and `gi-fx`. +- Fixed: console `promptPassword` threw `NullPointerException` at end of input; EOF is now cancellation and returns `null`, like every other console prompt. +- Fixed: `view(Matrix)` threw `NullPointerException` for a null matrix in all three implementations, and a matrix built without a name rendered the JavaFX tab title as `null (3 rows)`; unnamed matrices are now titled `Table`. +- Fixed: `Viewer`'s copy-shortcut OS check used a locale-sensitive `toLowerCase()`, which misbehaves on a Turkish-locale JVM. +- Fixed (packaging): fat JARs contained eight duplicated `LICENSE`, `NOTICE`, `DEPENDENCIES` and `buildinfo.xml` entries. They are now merged into one entry each, keeping every bundled dependency's licence text. +- Fixed (release): `release.sh` wrote generated notes above the curated `## Unreleased` section instead of promoting it, mangled commit subjects containing backslashes through `echo -e`, skipped the changelog entirely when `build.gradle` already held the release version, and could fail mid-release on a version that was not `MAJOR.MINOR.PATCH`. The changelog helpers now live in the sourceable `release-lib.sh`. +- Added: `urlExists` logs the cause it swallows at debug level; the boolean contract is unchanged. +- Added: both branches of the platform shell command are unit-tested, and a `windows-smoke` CI job runs the `gi-common` and `gi-console` suites on Windows. +- Removed: the `gi-fx` `rowsForViewer` identity helper. +``` + +- [x] **Step 2: Close the TODO items** + +In `TODO.md`, replace the `# Version 0.4.1` heading block's *Deferred* list with: + +```markdown +## Deferred + +- [x] Define the intended null-handling behavior for console `display(String)`. +- [x] Tidy the cosmetic Swing dialog title inconsistency. +- [x] Finish the root `test.groovy` sample update. +- [x] Review whether JavaFX `TableData` should remain publicly visible. +``` + +and add, directly under `## Review fixes (2026-09-25)`'s list: + +```markdown +## Review fixes (2026-09-26) +See `req/v0.4.1-fixes.md` for the plan and `release.md` for the changelog. +- [x] Build: Java 21 toolchain applied to every module +- [x] Build: Spotless and SpotBugs part of the default `check` +- [x] Build: OWASP scan covers all modules (`dependencyCheckAggregate`) +- [x] Build: JavaFX classifier resolved per OS *and* architecture +- [x] Console: null file name and EOF password are cancellation, not crashes +- [x] JavaFX/Swing/Console: null matrix guarded; unnamed matrices titled +- [x] Packaging: no duplicate licence/notice entries in the fat JARs +- [x] Release: `## Unreleased` promoted; version shape validated +- [x] Common: `urlExists` logs the cause; both shell platforms tested +``` + +- [x] **Step 3: Run the full verification set** + +Run: +```bash +./gradlew clean build +./check.sh +./gradlew -I /tmp/toolchain-probe.gradle -q help --no-configuration-cache 2>/dev/null | grep PROBE +./gradlew -q :gi-console:shadowJar :gi-swing:shadowJar :gi-fx:shadowJar +for m in gi-console gi-swing gi-fx; do + echo "== $m"; unzip -l $m/build/libs/$m-0.4.1-SNAPSHOT-fatjar.jar | awk 'NF>=4{print $4}' | sort | uniq -d +done +bash -n release.sh && bash -n release-lib.sh && echo "release scripts parse" +./release.sh --dry-run >/dev/null && git status --porcelain +``` +Expected: `BUILD SUCCESSFUL` twice; four `toolchain=21` probe lines; no duplicate paths for any module; `release scripts parse`; and an empty `git status --porcelain` apart from the files this task edits. + +- [x] **Step 4: Confirm the test count went up, not down** + +Run: +```bash +./gradlew test --rerun-tasks -q +grep -ho 'tests="[0-9]*"' */build/test-results/test/*.xml | awk -F'"' '{s+=$2} END {print "total tests: " s}' +``` +Expected: `BUILD SUCCESSFUL` and a total at least 10 higher than the pre-plan count (record the pre-plan number by running the same command on `main` before starting Task 1). + +- [x] **Step 5: Commit** + +```bash +git add release.md TODO.md +git commit -m "record the 2026-09-26 review fixes in the changelog + +Co-Authored-By: Claude Opus 5 " +``` + +- [x] **Step 6: Request a review of the finished branch** + +Use the `superpowers:requesting-code-review` skill against the full diff versus `main` before proposing a merge.