Skip to content

ADFA-5086: Fail a plugin command with bad paths or environment instead of throwing - #2091

Open
Daniel-ADFA wants to merge 2 commits into
stagefrom
bugfix/ADFA-5086-command-working-dir
Open

Daniel-ADFA wants to merge 2 commits into
stagefrom
bugfix/ADFA-5086-command-working-dir

Conversation

@Daniel-ADFA

@Daniel-ADFA Daniel-ADFA commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

ADFA-5086

IdeCommandServiceImpl.executeCommand no longer throws on bad plugin input. These now return an execution that has already completed with CommandResult.Failure:

  • workingDirectory with a NUL character (InvalidPathException from Paths.get)
  • workingDirectory too long to resolve (IOException from canonicalFile, at the resolve site and in validateWorkingDirectory)
  • environment names or values ProcessBuilder rejects (IllegalArgumentException, NUL or =)

Permission, concurrency and outside-the-project-root checks still throw as before. executable and arguments were already reported as a failure, since they only reach ProcessBuilder.start() inside the execution.

Review by commit: the first commit is a Spotless reformat of IdeCommandServiceImpl.kt only.

…owing

executeCommand passed the plugin-supplied ShellCommand.workingDirectory
straight to Paths.get and File.canonicalFile, so a NUL character threw
InvalidPathException and a path too long to resolve threw IOException,
both out of the host service and onto the plugin's calling thread. The
environment map had the same problem: ProcessBuilder rejects a NUL or
'=' in a name or value with IllegalArgumentException.

These now return a CommandExecution that has already completed with
CommandResult.Failure ("Invalid working directory: ..." or "Invalid
environment: ..."), the same shape a failed process start already
reports. Permission, concurrency and outside-the-project-root checks
still throw SecurityException/IllegalStateException as before.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Essentials

Run ID: 7646cace-3f4b-4e9f-815d-b5e7f20002fb

📥 Commits

Reviewing files that changed from the base of the PR and between 2a8eff5 and 6541d31.

📒 Files selected for processing (2)
  • plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeCommandServiceImpl.kt
  • plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeCommandServiceImplTest.kt

Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.


📝 Summary
  • executeCommand now returns a failed command execution when working-directory resolution or environment validation fails with InvalidPathException, IOException, or IllegalArgumentException. The failure includes exit code -1, the error message, and duration 0.
  • Added tests for a working directory with a NUL character, a working directory that is too long to resolve, and an environment value with a NUL character.
  • Risk: other failures, including SecurityException, are not converted to command failures and can still propagate. The supplied evidence does not include test-run results.

Walkthrough

Command execution now returns a rejected result when working-directory resolution or validation raises selected path or I/O errors, or when applying the environment raises IllegalArgumentException. Tests cover invalid working directories and an invalid environment value.

Changes

Command Input Rejection

Layer / File(s) Summary
Rejected command execution
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeCommandServiceImpl.kt, plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeCommandServiceImplTest.kt
executeCommand converts selected working-directory and environment errors into rejected executions. The rejected execution has empty output and returns a failure result with exit code -1 and duration 0. Tests cover a NUL-containing directory, a long directory, and a NUL-containing environment value.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 6541d

No actionable merge-blocking risk is established. The change reports invalid command setup as failures without changing the existing security checks; merge after normal checks pass.

🚥 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. 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 clearly and concisely describes the main change: plugin commands now fail instead of throwing when they receive invalid paths or environment values.
Description check ✅ Passed The description directly explains the affected inputs, resulting failure behavior, preserved exceptions, and test coverage. It is fully related to the changeset.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


A rabbit checks the paths with care,
Then tests the values tucked in there.
Bad inputs meet a quiet stop,
Empty output, no extra hop.
The command returns its failure note,
And hops away in one soft float.

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

@Daniel-ADFA
Daniel-ADFA requested a review from a team October 2, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants