[kixelated]

Problem

An exported gradle shell function makes command -v gradle return gradle instead of an executable path. Kotlin's check runner then calls its own gradle() wrapper recursively, and an explicit GRADLE_CMD=gradle recursed the same way. The packaging and bench runners run the inherited function instead of the executable.

This blocked the default FFI run in moq-dev/moq#7654, which needed a GRADLE_CMD override to pass.

Approach

All three Gradle runners (sh/kt/check.sh, sh/kt/package.sh, sh/kt/bench.sh) resolve GRADLE_CMD (default gradle) with Bash's executable-only type -P. Overrides still accept executable names and relative or absolute paths. When no executable resolves, the error names the value that failed, such as Gradle executable 'tools/missing-gradle' not found. It no longer says gradle is missing from PATH when the override was the problem.

sh/kt/gradle.test.py runs the real scripts in an isolated tree with an exported Gradle function. It covers defaults, overrides, missing executables and named overrides, failure propagation, and the check runner's credential-environment filtering. It is registered in the repository tooling gate (.github/justfile).

Impact

• Public API: none. Internal Gradle command selection now requires an executable.
• Wire: none.

Alternatives

• Renaming only the check wrapper would leave the package and bench runners selecting an inherited function. It would also leave executable discovery ambiguous. type -P never returns a function, wherever the wrapper is defined, so the wrapper keeps its name.
• A shared sourced helper for three five-line blocks would add a file and a fixture dependency without simplifying the callers.

Validation

On head 403bdd755f, after merging origin/main (no conflicts):

• python3 sh/kt/gradle.test.py: 5 tests pass in about 1 s. The fixture run against origin/main's runners fails 8 subcases.
• Reproduction: origin/main's sh/kt/check.sh, run with an exported gradle() { command gradle "$@"; } and no GRADLE_CMD, fails with gradle: maximum function nesting level exceeded. This head's script passes the same run, including real Gradle :moq-ffi:jvmTest :moq:jvmTest and the POM check.
• Default FFI run, no GRADLE_CMD override: just check HEAD with a comment-only rs/moq-ffi edit and the exported function, under one quest gate admission with LOCAL_SUPERVISED=1. It passed kt, c, cpp, go, dart, py, rs, obs_compile, and the other selected modules; Swift skips on Linux. No nesting errors.
• just check origin/main: shell, gh (including test gh), kt, markdown, and the other selected modules pass.
• Ruff lint and format, shellcheck, and shfmt pass on the changed files.

quest gate strips exported functions from the environment on this host. A plain just kt check therefore never saw the function. The runs above export it inside the admitted command, like the run in moq-dev/moq#7654.

Review

quest review --base origin/main --harness claude, Claude Code / claude-opus-5-5, medium effort. The code was authored by Codex, so Claude Code reviews it. Session 47ca2f5e-7915-4858-bed0-72b7800c055e.

• Round 1 on 3922cc4b18 had 3 findings:
  • sh/kt/bench.sh still ran a bare gradle: fixed in 403bdd755f. The fixture covers it.
  • A bad explicit GRADLE_CMD was reported as "gradle not on PATH": fixed in 403bdd755f. The error names the value, and test_missing_override_is_named covers it.
  • Rename the gradle() wrapper instead of relying on resolution order: refuted. type -P never returns a function, so there is no ordering invariant. The misleading comment is corrected in sh/kt/check.sh:30-31. The fixture also reproduces the package and override failures.
• Round 2 on 403bdd755f: no findings.

The PR had no review threads or automated findings to address.

Follow-ups

None.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 403bdd7

No actionable findings. All three runners resolve an executable before invoking it; the check wrapper uses that resolved command while retaining its environment filtering. The fixture covers the inherited function, explicit names and paths, missing executables, and failure propagation.

Verification limits: static GitHub source and test inspection only. I did not run Gradle, the fixture, or repository gates, and have not independently verified the reported checks.

(Written by review (OpenAI))

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The check, package, and benchmark scripts now resolve the configured Gradle command to an executable path. They report an error and exit with status 1 when resolution fails. A new unittest suite checks executable selection, overrides, missing commands, and failure-status handling. The justfile test recipe now runs this suite.

Merge Risk: Minimal · up to 403bd

No actionable merge-blocking risk is established. The change is mergeable subject to normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the main change: Gradle executables are resolved before wrapping them, which matches the type -P resolution in check.sh, package.sh, and bench.sh. It is specific and concise, and it us…
Description check		The description explains the problem (an exported gradle shell function causing recursion or running the function instead of an executable), the approach (type -P resolution of GRADLE_CMD in the three…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: Gradle executables are resolved before wrapping them, which matches the type -P resolution in check.sh, package.sh, and bench.sh. It is specific and concise, and it uses the conventional fix(kt) prefix. The changeset does not include any unrelated or misleading change.

Full details: Description check

Explanation

The description explains the problem (an exported gradle shell function causing recursion or running the function instead of an executable), the approach (type -P resolution of GRADLE_CMD in the three Kotlin runners), the new gradle.test.py suite, the improved error messages, and the validation performed. This matches the changeset closely.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: eccfbe41-b3a5-43dd-85f7-1ba83cb2243e

Commits

Reviewing files that changed from the base of the PR and between 2d1342e and 403bdd7.

Files selected for processing: 5

• .github/justfile
• sh/kt/bench.sh
• sh/kt/check.sh
• sh/kt/gradle.test.py
• sh/kt/package.sh

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

[pullfrog]

✅ No new issues found.

Reviewed changes

• Gradle resolution in the Kotlin runners: check.sh, package.sh, and bench.sh now resolve GRADLE_CMD (default gradle) with type -P, so an inherited gradle() function can no longer be selected. bench.sh also resolves before invoking Gradle instead of running a bare gradle.
• Named failure messages: each runner errors with the value it could not resolve (e.g. Gradle executable 'tools/missing-gradle' not found) rather than claiming Gradle is absent from PATH.
• Fixture: sh/kt/gradle.test.py runs the real scripts in an isolated tree with an exported Gradle function and covers defaults, explicit name/relative/absolute overrides, missing executables, failure propagation, and check.sh's credential-env filtering. Registered in the .github/justfile tooling gate.

I read the full diff, confirmed type -P behavior for bare names, relative/absolute paths, missing/non-executable files, and functions, ran python3 sh/kt/gradle.test.py (5 pass), and ran shellcheck on the three scripts. No other in-repo gradle call site shares this bug: the workflow steps have no inherited function, and test/smoke/smoke.sh's bare gradle has no same-name wrapper to recurse into.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}