Skip to content

build: upgrade to aesh 3.13 and fix primitive option types (#2504, #2…#2519

Merged
maxandersen merged 1 commit into
jbangdev:mainfrom
stalep:aesh-3.13
Jun 11, 2026
Merged

build: upgrade to aesh 3.13 and fix primitive option types (#2504, #2…#2519
maxandersen merged 1 commit into
jbangdev:mainfrom
stalep:aesh-3.13

Conversation

@stalep

@stalep stalep commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Upgrade aesh from 3.12.1 to 3.14.1 and readline-api from 3.11 to 3.14.
Leverage new aesh features to remove manual post-parse boilerplate.

aesh fixes used

Changes

DefaultValueProvider:

  • Add fallbackValue() to JBangDefaultValueProvider for config-based
    fallback on bare flags (--debug, --jfr, --open)
  • Split FALLBACK_ONLY_OPTIONS to skip config defaults for debug/jfr
    when the option is omitted (prevents always-on debug)

DebugConverter (new):

  • Converts raw --debug string directly into Map<String,String> via
    @Option(converter=...), replacing separate debugRaw/debugString fields

DebugOptionParser:

  • No longer sets empty sentinel for bare --debug; aesh's fallback
    chain (provider fallbackValue -> annotation fallbackValue) handles it

@OptionGroup for --javaagent:

  • Replace @OptionList + manual string splitting + lazy getter with
    @OptionGroup(defaultValue="") which handles key=value parsing,
    key-only entries, and insertion order natively

Boolean field cleanup:

  • Keep Boolean wrapper for negatable inherited flags (verbose, quiet,
    offline, fresh, preview, printExceptions) to support tri-state
    config override semantics (e.g. jbang config set verbose true
    overridden with --no-verbose on the CLI)
  • Convert non-negatable options to primitive boolean where null
    semantics aren't needed (insecure, enablePreviewRequested, etc.)

Env var defaults via ${env:VAR:-} (aesh#514):

  • AIOptions: defaultValue = "${env:JBANG_AI_*:-}" replaces manual
    System.getenv() lookups in Init.afterParse()

Other simplifications:

  • Field initializers for userParams (List<String> = new ArrayList<>())
  • Remove resolveAfterParse() calls from Run, App, Alias
  • Remove redundant config fallback from Edit.applyEditorDefaults()
  • Remove empty afterParse() overrides

Co-authored-by: Max Rydahl Andersen max@xam.dk

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3269c36b-afc6-44f5-806b-19c120090a7d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR upgrades the org.aesh dependency to version 3.13, refactors the default value provider to separate fallback-only options (debug, jfr) from standard defaults, converts several Boolean options to primitive booleans, adds null-safe checks for nullable options, and integrates explicit fallback value resolution in RunMixin.

Changes

Option parsing and defaults refactoring

Layer / File(s) Summary
Dependency upgrade to org.aesh 3.13
build.gradle
org.aesh implementation and processor dependencies upgraded from 3.12.1 to 3.13.
Boolean-to-primitive conversions and null-safe checks
src/main/java/dev/jbang/cli/Alias.java, src/main/java/dev/jbang/cli/Catalog.java, src/main/java/dev/jbang/cli/Jdk.java, src/main/java/dev/jbang/cli/BaseCommand.java
enablePreviewRequested, importItems, and isDefault fields converted from nullable Boolean to primitive boolean. BaseCommand applies null-safe Boolean.TRUE.equals() guard for insecure SSL initialization.
Default value provider refactoring with FALLBACK_ONLY_OPTIONS
src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
Introduces FALLBACK_ONLY_OPTIONS set containing debug and jfr. The defaultValue() method now returns null for these options. A new fallbackValue() override method resolves values from configuration using fully-qualified command-path-prefixed keys when available, falling back to unqualified option names.
Debug option fallback integration
src/main/java/dev/jbang/cli/RunMixin.java
The debug option annotation specifies fallbackValue="4004". The resolveAfterParse() method performs Configuration lookup with explicit default instead of cross-option fallback from flightRecorderString.
Template output formatting
src/main/java/dev/jbang/cli/Template.java
Reformatted printTemplatesWithOrigin() call arguments across multiple lines with no logic changes.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main changes: upgrading aesh to 3.13 and fixing primitive option types, with issue references.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
@maxandersen maxandersen added the ai-review used to explicitly request ai-review label Jun 8, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/main/java/dev/jbang/cli/BaseCommand.java (1)

42-42: ⚡ Quick win

Consider changing insecure to Boolean wrapper for consistency.

The insecure field is a primitive boolean, but all other inherited boolean flags in this class (verbose, quiet, offline, fresh, preview, printExceptions) use the Boolean wrapper type to support tri-state semantics (unset/true/false). Although insecure is not marked as negatable or inherited, the null-safe check at line 193 (Boolean.TRUE.equals(insecure)) suggests wrapper semantics may be intended. If insecure should remain primitive, the check can be simplified to if (insecure).

♻️ If wrapper type is intended, apply this change
-	boolean insecure;
+	Boolean insecure;

Otherwise, simplify the null-safe check at line 193:

-		if (Boolean.TRUE.equals(insecure)) {
+		if (insecure) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/dev/jbang/cli/BaseCommand.java` at line 42, Change the
primitive boolean field insecure in class BaseCommand to the Boolean wrapper to
match the tri-state semantics of other flags (verbose/quiet/offline/etc.);
update the field declaration from boolean insecure to Boolean insecure and leave
the null-safe check using Boolean.TRUE.equals(insecure) intact (and scan for
other direct boolean uses of insecure elsewhere in BaseCommand to handle
possible nulls).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/main/java/dev/jbang/cli/BaseCommand.java`:
- Line 42: Change the primitive boolean field insecure in class BaseCommand to
the Boolean wrapper to match the tri-state semantics of other flags
(verbose/quiet/offline/etc.); update the field declaration from boolean insecure
to Boolean insecure and leave the null-safe check using
Boolean.TRUE.equals(insecure) intact (and scan for other direct boolean uses of
insecure elsewhere in BaseCommand to handle possible nulls).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 59fcfd42-9a6e-4b1c-aa02-75f33d728e3e

📥 Commits

Reviewing files that changed from the base of the PR and between 31fa8d5 and 811f8e6.

📒 Files selected for processing (8)
  • build.gradle
  • src/main/java/dev/jbang/cli/Alias.java
  • src/main/java/dev/jbang/cli/BaseCommand.java
  • src/main/java/dev/jbang/cli/Catalog.java
  • src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
  • src/main/java/dev/jbang/cli/Jdk.java
  • src/main/java/dev/jbang/cli/RunMixin.java
  • src/main/java/dev/jbang/cli/Template.java

@maxandersen

Copy link
Copy Markdown
Collaborator

@stalep — Use aesh 3.14.1's ${env:VAR:-fallback} variable resolution (aeshell/aesh#514) to declare env var defaults directly in @Option annotations, replacing manual System.getenv() lookups in afterParse().

Changes:

  • AIOptions: Add defaultValue = "${env:JBANG_AI_*:-}" to --ai-provider, --ai-api-key, --ai-endpoint, --ai-model
  • Edit: Add defaultValue/fallbackValue = "${env:JBANG_EDITOR:-}" to --open, remove applyEditorDefaults()
  • Init: Remove afterParse() override that manually checked JBANG_AI_* env vars

This brings env var handling in line with how picocli did it (e.g. defaultValue = "${JBANG_EDITOR:-${default.edit.open:-}}") — declarative in annotations rather than imperative post-parse code. Config-based defaults are still handled by JBangDefaultValueProvider.

@stalep

stalep commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Looks good! 👍

@quintesse

Copy link
Copy Markdown
Contributor

can now be overridden with '--no-force' on the CLI.

And with --force=false, I assume, right?

@stalep

stalep commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

No, --force=false and --no-force don't work.
The force options are hasValue = false and not negatable. The commit message example mentioning --no-force was wrong — the negatable override pattern only applies to the inherited boolean flags (verbose, quiet, offline, fresh, preview) which are declared negatable = true with Boolean wrapper type.
A correct example would be: jbang config set verbose true overridden with --no-verbose. I've fixed the commit message.

@quintesse

quintesse commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

the negatable override pattern only applies to the inherited boolean flags (verbose, quiet, offline, fresh, preview)

What is your opinion about that @maxandersen ? Because it's definitely a breaking change. When we talked about this before we only mentioned those options because that's what was visible at that point in the code, but that syntax of booleanOpt=true/false is applicable to any boolean in Picocli,

So are we okay with not being backward compatible? If so (we're not at JBang 1.0.0 afterall) do we even want to special case those 5 options? Or do we go the other way and try to be 100% backward compatible?

Edit: see next message

@quintesse

quintesse commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

No, --force=false and --no-force don't work.
the negatable override pattern only applies to the inherited boolean flags

Well the issue is that ALL options have config overrides in JBang, not only those 5 global options. So how can I ever disable anything that was enabled by the user's config?

A correct example would be: jbang config set verbose true overridden with --no-verbose

And here my question is again: "and with --verbose=false right?" Otherwise we're still not backward compatible. (Although we could perhaps have a discussion about breaking compatibility once and go for this new syntax, but that will depend a lot on @maxandersen , personally I'm not a proponent of breaking things unnecessarily)

@stalep

stalep commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Good news with the new version of aesh (3.14.2): --flag=false works in aesh for all boolean options, even those with hasValue = false and without negatable = true. So --force=false already works to override a config-set force = true.

Verified:

  • --verbose=true / --verbose=false -- works (negatable option)
  • --force=true / --force=false -- works (non-negatable, hasValue = false)

So the =true/=false syntax is backward compatible with picocli. The --no-* syntax is only available for options declared negatable = true (the inherited global flags), but the =false syntax works universally.

maxandersen
maxandersen previously approved these changes Jun 11, 2026
@maxandersen

Copy link
Copy Markdown
Collaborator

merging in as its doing all the right things. lets fix anything remaining/new in separate prs

Upgrade aesh from 3.12.1 to 3.14.2 and readline-api from 3.11 to 3.14.
Leverage new aesh features to remove manual post-parse boilerplate.

aesh fixes used:
- jbangdev#511: Fallback value chain runs after custom OptionParser
- jbangdev#512: ${env:...} resolution on annotation processor path
- jbangdev#513: LinkedHashMap for @OptionGroup (preserves insertion order)
- jbangdev#514: Cascading ${key:-fallback} variable resolution
- jbangdev#520: Deferred ${env:...} resolution to parse time (testability)
- jbangdev#521: Annotation defaultValue with resolved env var takes priority
  over DefaultValueProvider

DefaultValueProvider:
- Add fallbackValue() to JBangDefaultValueProvider for config-based
  fallback on bare flags (--debug, --jfr, --open)
- Split FALLBACK_ONLY_OPTIONS to skip config defaults for debug/jfr
  when the option is omitted (prevents always-on debug)

DebugConverter (new):
- Converts raw --debug string directly into Map<String,String> via
  @option(converter=...), replacing separate debugRaw/debugString fields

DebugOptionParser:
- No longer sets empty sentinel for bare --debug; aesh's fallback
  chain (provider fallbackValue -> annotation fallbackValue) handles it

@OptionGroup for --javaagent:
- Replace @OptionList + manual string splitting + lazy getter with
  @OptionGroup(defaultValue="") which handles key=value parsing,
  key-only entries, and insertion order natively

Env var defaults via ${env:VAR:-} (aesh#514, jbangdev#520, jbangdev#521):
- AIOptions: defaultValue = "${env:JBANG_AI_*:-}" replaces manual
  System.getenv() lookups in Init.afterParse()
- Edit --open: defaultValue/fallbackValue = "${env:JBANG_EDITOR:-}"
  replaces manual applyEditorDefaults(). Env var takes priority over
  config (aesh#521), config provides fallback when env var is unset.

Boolean field cleanup:
- Keep Boolean wrapper for negatable inherited flags (verbose, quiet,
  offline, fresh, preview, printExceptions) to support tri-state
  config override (e.g. config set verbose true, override with
  --no-verbose)
- Convert non-negatable options to primitive boolean where null
  semantics aren't needed (insecure, enablePreviewRequested, etc.)

Other simplifications:
- Field initializers for userParams (List<String> = new ArrayList<>())
- Remove resolveAfterParse() calls from Run, App, Alias
- Remove empty afterParse() overrides

Co-authored-by: Max Rydahl Andersen <max@xam.dk>
@maxandersen maxandersen merged commit 149352f into jbangdev:main Jun 11, 2026
54 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review used to explicitly request ai-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants