Skip to content

Adding ktlint to Test Improvers PRs#3718

Open
Jignesh-dimagi wants to merge 1 commit into
masterfrom
ktlint-pr-comment-handler
Open

Adding ktlint to Test Improvers PRs#3718
Jignesh-dimagi wants to merge 1 commit into
masterfrom
ktlint-pr-comment-handler

Conversation

@Jignesh-dimagi

Copy link
Copy Markdown
Contributor

Problem:

In PR #3614, the PR Comment Handler bot was asked to format a Kotlin file via ./gradlew ktlintFile and replied that it couldn't.

The Gradle wrapper cannot write its lock file under ~/.gradle. The bot runs inside gh-aw's sandboxed container where $HOME is read-only, so the Gradle wrapper's default user-home directory (~/.gradle/wrapper/dists/.../*.lock) is unwritable. Result: any ./gradlew invocation by either the PR Comment Handler or the Daily Test Improver fails.

Fix:

Edited two workflow prompts (pr-comment-handler.md, daily-test-improver.md) to:

  • Added a "Running Gradle Tasks" appendix to each file telling the agent to set GRADLE_USER_HOME=$GITHUB_WORKSPACE/.gradle-home inline on every ./gradlew call
  • In daily-test-improver: added an explicit Step 5.e: "Run ktlint per .kt file" — after finishing edits to each Kotlin file, run ./gradlew ktlintFile -PfilePath=<path> on that file.
  • In pr-comment-handler: cross-referenced thisRunning Gradle Tasks from Step 5.3, to run ./gradlew ktlintFile -PfilePath=<path> for kotlin file

What to expect in future Test Improver PRs:

  • Kotlin files will be ktlint-formatted in the PR itself.
  • Reviewer requests to ktlint a file will succeed — PR Comment Handler can now run the gradle task when asked.

This PR does not fix:

  • PRs from the Test Improver have been shipping with tests it never actually ran due to issue: missing ../commcare. This PR doesn't fix this issue.
  • PRs will continue to include a Test Status note that tests require the commcare-core sibling-directory

Note:

No .lock.yml regeneration needed - Lock files only need regenerating when we change the .md frontmatter (network allowlist, tools, schedule, safe-outputs, etc.). Pure prompt-body edits like this PR don't require it.

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7bbadef9-e9f1-4440-acee-12c9e5986e47

📥 Commits

Reviewing files that changed from the base of the PR and between 77526ef and 40f4c9c.

📒 Files selected for processing (2)
  • .github/workflows/daily-test-improver.md
  • .github/workflows/pr-comment-handler.md

📝 Walkthrough

Walkthrough

This PR updates two workflow documentation files (.github/workflows/) to standardize guidance on Kotlin formatting and Gradle execution within sandbox constraints. The daily-test-improver.md file now specifies that Task 3 should run ktlintFile per edited .kt file, and pr-comment-handler.md refines linting instructions to route only Kotlin files through ktlintFile while keeping Java validation separate. Both files add new "Running Gradle Tasks" sections explaining that the sandbox's read-only default Gradle cache requires setting GRADLE_USER_HOME inline for every ./gradlew call, and noting resource/timeout limitations.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

  • [Test Improver] Monthly Activity 2026-04 #3642: Both PRs update the same workflow documentation sections for Kotlin ktlintFile formatting (per-file invocation) and document the Gradle sandbox GRADLE_USER_HOME configuration requirement.

Suggested reviewers

  • conroy-ricketts
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes a Problem, Fix, and expected outcomes, but lacks all required template sections: Product Description, Safety Assurance, automated test coverage, and mandatory checklist items. Add the missing template sections: Product Description (user-facing effects), Safety Assurance with safety story and test coverage details, and complete the checklist (QA testing, release notes, risk label, reviewers).
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: adding ktlint formatting capability to Test Improver PRs by enabling Gradle task execution in the sandboxed environment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ktlint-pr-comment-handler

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 23.34%. Comparing base (86fe3ec) to head (40f4c9c).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3718      +/-   ##
============================================
- Coverage     23.35%   23.34%   -0.02%     
+ Complexity     3927     3925       -2     
============================================
  Files           923      923              
  Lines         56058    56058              
  Branches       6641     6641              
============================================
- Hits          13093    13084       -9     
- Misses        41271    41282      +11     
+ Partials       1694     1692       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

think we need to have these prompts quite small for them to be not costly, I would suggest just adding a single line ktlint all changed files as one sentence in both the files and testing whether the agent is able to figure out the rest of details by itself as a first iteration here.

@conroy-ricketts conroy-ricketts 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.

I have no additional concerns

@Jignesh-dimagi

Copy link
Copy Markdown
Contributor Author

@shubham1g5 @OrangeAndGreen @conroy-ricketts
I think AI has gone ahead and tried to solve this issue. As you can check here, ktlint was not available till the last run, but it has been installed now by itself. Now, we have ktlint available in GitHub!!! I have given comments on other Test Improver PRs for ktlint; let's see if that works.

Question: Do we still want this ktlintFile gradle task to be working? I don't see any advantage if ktlint is installed now.

@shubham1g5

Copy link
Copy Markdown
Contributor

Do we still want this ktlintFile gradle task to be working?

yeah, think it's still good to rely on it, the idea is for AI agents or humans to be able to use it when they need to ktlint (locally or on an external host)

@Jignesh-dimagi

Copy link
Copy Markdown
Contributor Author

think we need to have these prompts quite small for them to be not costly, I would suggest just adding a single line ktlint all changed files as one sentence in both the files and testing whether the agent is able to figure out the rest of details by itself as a first iteration here.

I am not sure if I understand your comment here. Do you mean here that we should remove 'Running Gradle Tasks' from the appendix and give the command 'ktlint all changed files'?

@shubham1g5

Copy link
Copy Markdown
Contributor

I am not sure if I understand your comment here. Do you mean here that we should remove 'Running Gradle Tasks' from the appendix and give the command 'ktlint all changed files'?

yeah I just meant replacing all additions with the single line that asks it to do Ktlint and relying on the agent to know it knows how to do it in project context using AGENT.md

But if KTlint is already happening on new PRs, think we don't need to take any action here at all and close this PR without merge.

@Jignesh-dimagi

Copy link
Copy Markdown
Contributor Author

yeah I just meant replacing all additions with the single line that asks it to do Ktlint and relying on the agent to know it knows how to do it in project context using AGENT.md

Now, with ktlint being installed already on GitHub, we can try for that simple command, but I have already given comments here and here to the ktlint file, but there has been no action so far. I am waiting to get some proof that it can run ktlint without a Gradle task.

But if KTlint is already happening on new PRs, think we don't need to take any action here at all and close this PR without merge.

I think we will still require this PR, as daily-test-improver and pr-comment-handler don't have any mention of ktlint.

@shubham1g5

Copy link
Copy Markdown
Contributor

I think we will still require this PR, as daily-test-improver and pr-comment-handler don't have any mention of ktlint.

They read AGENTS.md which mention that. Think this issue was only on old PRs when we didn't mention AGENTS.md in these files. Anyway, I think it's good to check if there is a problem on new PRs without merging this and only make a change if so.

@conroy-ricketts

Copy link
Copy Markdown
Contributor

@Jignesh-dimagi it's taking a long time for the GitHub bot to update the Test Improver PRs I think. Are you ok with me using Claude to close out these PRs and we can watch for ktlint on new PRs in the future? They've been sitting for a while. I started using Claude on one of these PRs and then realized after that this PR exists

@Jignesh-dimagi

Copy link
Copy Markdown
Contributor Author

@Jignesh-dimagi it's taking a long time for the GitHub bot to update the Test Improver PRs I think. Are you ok with me using Claude to close out these PRs and we can watch for ktlint on new PRs in the future? They've been sitting for a while. I started using Claude on one of these PRs and then realized after that this PR exists

Yes, you can go ahead with other PRs, as they are not related to this one. However, the ktlint GitHub bot doesn't seem to be responding, at least for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants