Skip to content

fix: default OOP tool workspaces to the BOSS root - #28

Draft
shivanshu-risa wants to merge 6 commits into
mainfrom
fix/boss-data-root
Draft

shivanshu-risa wants to merge 6 commits into
mainfrom
fix/boss-data-root

Conversation

@shivanshu-risa

@shivanshu-risa shivanshu-risa commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Default generated Tool Creator workspaces to ~/.boss/workspaces/tools.
  • Keep holder startup non-fatal when the home is blank, blocked, read-only, or symlinked; the form surfaces unavailable locations instead of killing the plugin UI.
  • Preserve explicit project and SetParentDir locations as user-selected output paths. This PR changes the default, not the user's ability to choose a project directory.
  • Pin CI/release contract downloads to the compatible BossConsole release instead of following latest.
  • Reject an ipc_version release override unless its matching BossConsole release tag is supplied.
  • Keep release workflow guidance and the API-jar fallback message aligned with the new defaults. Default users now source the API jar from ~/.boss/plugins; explicit custom parent directories retain the sibling-build lookup.

Verification

  • CI=true ./gradlew clean build --no-build-cache --no-daemon (pass, 261 tests)
  • Mutation check: temporarily restoring the throwing directory check makes unavailable boss path does not prevent initial state publication fail; restoring the fix makes the clean suite pass.
  • CI=true ./gradlew downloadDeps -Pbossconsole.release.tag= --rerun-tasks --no-daemon fails with the intended actionable property error.

v9.5.19 is the pinned compatible release for IPC 1.0.0; newer BossConsole releases publish newer IPC asset names.

@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: keep OOP tool workspaces under the BOSS root

Moving the default away from the hard-coded ~/Development/Boss/boss_plugins is a good change, and splitting out defaultParentDir(userHome) makes it testable. I found one blocking problem and a few smaller ones.

🔴 The constructor can now throw, so the holder never publishes

defaultParentDir() is evaluated in the constructor delegation (ToolCreatorStateHolder.kt:249 and :252), before super(...) and before the first updateState. The new body can throw in three ways:

  • check(tools.exists() || tools.mkdirs()) fails on a read-only or quota-limited home, or when user.home is blank (File("", ".boss") resolves to /.boss, which an ordinary user can't create).
  • require(startsWith(bossRoot)) fails when ~/.boss/workspaces or ~/.boss/workspaces/tools is a symlink pointing outside ~/.boss. That's a reasonable setup, for example workspaces kept on another volume.
  • canonicalFile can throw IOException.

The old code could never throw: it fell back to user.home. If construction fails now, the child dies in PluginProcessMain (or the holder is never created), and the Tool Creator surface renders nothing. AGENTS.md calls that out as the failure mode every holder must avoid. It also goes against the "never crash on a missing provider/environment" convention.

Suggestion: never throw from this path. Fall back to the previous behaviour, then let the existing validate() show "Location does not exist" in the form if needed:

internal fun defaultParentDir(userHome: String): String {
    if (userHome.isBlank()) return ""
    return runCatching {
        val bossRoot = File(userHome, ".boss").canonicalFile
        val tools = File(bossRoot, "workspaces/tools").canonicalFile
        if (!tools.toPath().startsWith(bossRoot.toPath())) return@runCatching null
        tools.takeIf { it.isDirectory || it.mkdirs() }?.absolutePath
    }.getOrNull() ?: userHome
}

(Whether a symlinked workspace should be rejected at all is a separate question. Rejecting it is defensible, but it should degrade, not crash.)

🟠 The containment guarantee only covers the default

The title says workspaces are kept under the BOSS root, but that's only true for the default path:

  • RemotePluginContext.projectPath?.let { File(it).parent } (:252) still wins when a project is open, and it isn't checked.
  • SetParentDir (:281) accepts any path, and validate() only checks isDirectory.

If containment is a real security or invariant requirement (does the in-process plugin enforce it?), it belongs in validate()/derive() so it covers every source of parentDir. If it's only a nicer default, the require is mostly a symlink tripwire, and the PR description should say so.

🟠 Behaviour regression in copyApiJar

copyApiJar (:480) looks first in File(form.parentDir, "boss-plugin-api/build/libs"). That sibling lookup only made sense because the old default was the boss_plugins checkout directory. Under ~/.boss/workspaces/tools the candidate never exists, so developers with a local boss-plugin-api build fall back to ~/.boss_debug/plugins / ~/.boss/plugins, and possibly an older jar. That may be acceptable, but it should be a deliberate choice. Consider keeping ~/Development/Boss/boss_plugins/boss-plugin-api/build/libs as an explicit candidate, or updating the warning text ("local builds need ../boss-plugin-api"), which no longer matches the default layout.

🟡 Side effects on construction, including in tests

mkdirs() now runs every time the holder is constructed, even if the user never creates a tool. The existing tests that call ToolCreatorStateHolder(scope) (e.g. lines 57, 65, 79, 85…, and IntentDeserializerArmsTest:78) now create ~/.boss/workspaces/tools in the real home of whoever runs ./gradlew test, including CI. Options:

  • create the directory lazily, in StartBuild, or
  • have the tests set user.home to a temp dir, or inject the home.

🟡 Test nits

  • The temp home from Files.createTempDirectory is never deleted. Use try/finally { home.toFile().deleteRecursively() }, or the existing @AfterTest.
  • java.nio.file.Path is fully qualified inline. Import it next to Files. Also, the imports are already out of order (java.nio.file.Files lands after the kotlin.test block).
  • There's a stray double blank line after the new test.
  • The test covers the happy path only. Please add cases for: a symlinked workspaces escaping the root (once it degrades instead of throwing, assert the fallback), a blank userHome, and an unwritable home. Those are the cases that would have caught the constructor crash.
  • assertTrue(workspace.startsWith(expectedRoot)) is implied by the assertEquals just above it.

✅ Conventions

  • No Android APIs, no version bumps, and no plugin.json edits. 👍
  • Both Kotlin files end with a newline.
  • The holder still publishes from its constructor, as long as the constructor gets that far (see the blocking issue).

Summary

The direction is right. Please make defaultParentDir non-throwing before merging, since today a bad home directory or a symlinked workspace turns into a dead plugin with no UI. The scope of the containment guarantee and the copyApiJar lookup are worth a decision in this PR too.

@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: fix: keep OOP tool workspaces under the BOSS root

Thanks for this. Pinning the upstream release instead of following /releases/latest is the right call, and moving generated tools out of ~/Development/Boss/boss_plugins makes sense. I found one blocking issue and a few smaller ones.

🔴 Blocking: defaultParentDir can now throw inside the holder constructor

ToolCreatorStateHolder.kt:593-599 uses require(...) and check(...), and defaultParentDir() runs while the constructor arguments are evaluated (:249, and :252 whenever projectPath is null). That happens before super(...) and the first updateState. So if the directory can't be created (read-only home, empty user.home, a file sitting at ~/.boss/workspaces, permissions), construction throws. Per AGENTS.md, the plugin then never publishes, so it never renders.

The old code always fell back to some path. The holder already handles a bad location gracefully: validate() reports "Location does not exist: …" (:636). I'd suggest degrading instead of throwing:

internal fun defaultParentDir(userHome: String): String {
    val tools = File(userHome, ".boss/workspaces/tools")
    if (!tools.isDirectory && !tools.mkdirs()) {
        LoggerFactory.getLogger(ToolCreatorStateHolder::class.java)
            .warn("Could not create BOSS tool workspace {}", tools)
    }
    return tools.absolutePath
}

A test with an uncreatable home (for example a regular file where .boss should be) asserting that the holder still constructs and publishes ready = true would pin this down.

🟠 The containment require checks a constant, and it can break legitimate setups

  • workspaces/tools is a hard-coded literal, so the only way tools can "escape" bossRoot after canonicalisation is a symlink. Some people symlink ~/.boss/workspaces to another disk on purpose. With this change, their Tool Creator crashes at startup (see above) instead of working.
  • The check also doesn't cover what users actually control: SetParentDir (:281) still accepts any path, and the projectPath-derived default (:252) isn't checked either. So the PR description's "assert containment beneath the BOSS root" is broader than what the code does.

I'd drop the require, or limit it to rejecting .. in a configurable segment if one is ever added. If containment really is a security requirement, it belongs in validate() so it applies to every location.

🟡 Behaviour change in copyApiJar

copyApiJar looks in File(form.parentDir, "boss-plugin-api/build/libs") first (:480). With the old default (boss_plugins), that found a developer's freshly built sibling API jar. Under ~/.boss/workspaces/tools it never will, so scaffolds now get whatever is installed in ~/.boss/plugins, which may be a different version. The log hint at :490 ("need ../boss-plugin-api") is also stale for the new location. Is this intentional, and does the in-process plugin behave the same way? If so, please update the log message to match.

🟡 release.yml is now out of sync

  • The bossconsole_release_tag input description still says (default: latest) (release.yml:47).
  • The comment at release.yml:126-127 still says that leaving it blank falls back to /releases/latest/download. It now falls back to bossconsole.release.tag.

Please update both so whoever runs the next release isn't misled.

🟡 Missing property fails obscurely

If bossconsole.release.tag is ever removed or misspelled, source has no value and downloadDeps fails with a generic "provider has no value" error. Something like .orElse(provider { error("Set bossconsole.release.tag in gradle.properties or pass -Pupstream.source") }), or an explicit check in doLast, would make that failure self-explanatory.

Tests / style nits

  • The new test never deletes its temp home. The other tests in this file call deleteRecursively(), so please wrap this one in try/finally too.
  • There's a double blank line after the new test.
  • java.nio.file.Files is imported out of order (after kotlin.test.*), and java.nio.file.Path is fully qualified inline. Import both alongside java.io.File.
  • AGENTS.md says 254 tests. Please bump the count if it's maintained.

Conventions checklist

  • ✅ No version changes. plugin.json is untouched.
  • ✅ Both Kotlin files end with a newline.
  • ✅ No Android or Compose APIs involved.
  • ⚠️ Null-safe provider access is fine (context.projectPath?.let), but the new throwing default undermines the same "never crash on startup" intent (see the blocking item).

Summary

The Gradle/CI pinning is good to merge once release.yml is updated to match. For the state-holder change, please make defaultParentDir non-throwing before merging, and reconsider the symlink-hostile require.

@shivanshu-risa shivanshu-risa changed the title fix: keep OOP tool workspaces under the BOSS root fix: default OOP tool workspaces to the BOSS root Oct 9, 2026
@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: default OOP tool workspaces to the BOSS root

This is a well-scoped PR, and the reasoning is written down where the next person will look for it. The non-fatal defaultParentDir matters because it runs inside this(scope, defaultParentDir()), before the private constructor's first updateState. A throw there would hit the "version 0, renders nothing" failure that AGENTS.md describes. Your mutation check on unavailable boss path does not prevent initial state publication shows the test actually covers that. Pinning the contract download instead of following /releases/latest is the right call too.

Potential issues

  1. Overriding ipc_version alone in the release workflow now fails with a 404. release.yml still offers an ipc_version input. If someone sets it to e.g. 1.1.0 and leaves bossconsole_release_tag blank, downloadDeps asks v9.5.19 for boss-ipc-1.1.0.jar. That asset doesn't exist, so conn.getInputStream() throws a raw FileNotFoundException whose message is just the URL. Before this PR, latest would at least have matched a newer IPC version. Some options:

    • Make the input description say the two inputs must change together, or fail the step early when ipc_version is set and the tag is blank.
    • Catch the FileNotFoundException in downloadDeps and rethrow a GradleException like "<jar> not published in <tag>; bossconsole.release.tag and ipc.version must match".
  2. The boss-plugin-api sibling lookup no longer finds anything for default users. copyApiJar still checks File(form.parentDir, "boss-plugin-api/build/libs") first. With the old default (~/Development/Boss/boss_plugins), developers had a checkout there. Under ~/.boss/workspaces/tools it won't exist, so those users silently fall through to ~/.boss_debug/plugins / ~/.boss/plugins. If that's intended (the updated warning suggests it is), a short note in the PR body or AGENTS.md would help. Otherwise, consider keeping ~/Development/Boss/boss_plugins/boss-plugin-api/build/libs as an explicit extra candidate so local API builds are still picked up.

  3. Building the holder now creates a directory. defaultParentDir() calls mkdirs(), so every Tool Creator launch without a project context creates ~/.boss/workspaces/tools, even if the user never generates anything. It's harmless and it makes the "Location does not exist" validation pass on first open, so I think it's fine. It's just a new side effect, and a one-line comment would cover it.

Code quality / nits

  • build.gradle.kts, downloadDeps: the same error message is thrown from two places, the map {} and the doLast fallback. A shorter form keeps one source of truth:
    val source = providers.gradleProperty("upstream.source").orElse(
        providers.gradleProperty("bossconsole.release.tag")
            .map(String::trim).filter { it.isNotEmpty() }
            .map { "https://github.com/risa-labs-inc/BossConsole-Releases/releases/download/$it" }
    )
    // doLast: source.orNull ?: throw GradleException("Set bossconsole.release.tag ... or pass -Pupstream.source")
    (Provider.filter needs Gradle 8.5+; if the wrapper is older, the current form is fine.)
  • ToolCreatorStateHolder.defaultParentDir: runCatching { toolsPath.absoluteFile } can only fail on a SecurityException, so that branch is effectively dead. It would read more simply as a single runCatching around the isDirectory/mkdirs block. The three LoggerFactory.getLogger(...) calls could also become one local val log.
  • Always wiring downloadApiContract: this is a good fix for clean build. One related detail: bossPluginApiJar itself is still chosen at configuration time with firstOrNull { it.exists() }, so the same clean-ordering problem could in theory affect which candidate gets picked. In practice the sibling candidates aren't under build/ of this project, so clean doesn't touch them. Just mentioning it.
  • Test import reorder (StateWireJson): good cleanup.

Tests

  • The four new tests cover the cases that matter: the happy path, blank home, a blocked .boss (a regular file, which also fails correctly when CI runs as root), and a symlinked workspaces.
  • Symlink test cleanup: home.toFile().deleteRecursively() follows the workspaces symlink into external and empties it before external is deleted. That's harmless here, but deleting external first, or using Files.walk with NOFOLLOW_LINKS, would avoid following links during cleanup.
  • Missing test: nothing checks that the RemotePluginContext constructor still prefers projectPath's parent over the new default. The PR body says this behaviour is preserved, so a small test would lock it in.
  • The AGENTS.md count (254 → 260) matches the new tests.

Security

  • No new risk. The default stays under the user's home, and the network fetch is now pinned to a tag instead of a moving latest, which is better for supply-chain stability.
  • Possible follow-up: verify the downloaded contract jars against known SHA-256s, since downloadDeps currently trusts whatever the release asset serves.

BOSS conventions

  • Compose / Android APIs: none used.
  • PluginContext providers: the context.projectPath?.let { ... } ?: defaultParentDir() path is still null-safe.
  • Runtime version: not touched; only ipc.version neighbours and a new property were added to gradle.properties, which is fine.
  • Trailing newlines: I couldn't check them from the diff (no "No newline at end of file" markers appear, so they look fine).

Overall I'd approve once item 1 is handled, at least in the input description, since the others are nits. Thanks for the thorough verification notes.

@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: default OOP tool workspaces to the BOSS root

Overall this looks good to merge. The main fix is right. defaultParentDir runs before the holder's first updateState, and if it threw there the holder would stay at version 0 and the host would render nothing (see AGENTS.md → stateholders/). Degrading into the form's existing Location does not exist validation keeps the plugin visible. The new test (unavailable boss path does not prevent initial state publication) asserts version > 0 directly, and the mutation check in the description shows it catches the regression. Pinning CI to v9.5.19 instead of /releases/latest is also the right call.

Possible issues

  1. Script injection in release.yml (low severity, partly pre-existing). UPSTREAM_TAG="${{ inputs.bossconsole_release_tag }}" (and the existing IPC_VERSION="${{ inputs.ipc_version }}") put workflow inputs straight into the shell script. Only people with write access can dispatch this workflow, so the risk is small. The Build fatJar step already passes the tag through env:, though, so the same pattern would make this step consistent and safe:

    - name: Resolve versions
      env:
        IPC_VERSION_INPUT: ${{ inputs.ipc_version }}
        UPSTREAM_TAG: ${{ inputs.bossconsole_release_tag }}

    Then read $IPC_VERSION_INPUT and $UPSTREAM_TAG in the script.

  2. The default sibling-jar lookup can no longer find anything. copyApiJar still checks File(form.parentDir, "boss-plugin-api/build/libs") first. With the new default this becomes ~/.boss/workspaces/tools/boss-plugin-api/build/libs, which won't exist. That's harmless, and the description says it's intended. It does mean developers who used to get ~/Development/Boss/boss_plugins automatically now silently fall back to whatever is in ~/.boss/plugins, which might be an older jar than their local build. A short note in the release notes or a one-line comment above the candidate list would help. The new warning text also drops ~/.boss_debug/plugins, even though it's still searched. Consider listing it as well.

  3. Opening the plugin now writes to disk. defaultParentDir calls mkdirs() from the constructor, so just opening Tool Creator creates ~/.boss/workspaces/tools. That's reasonable under ~/.boss and the comment explains it. I'm mentioning it because the old code was read-only at startup.

Code quality

  • runCatching { toolsPath.absoluteFile } is very defensive. File.getAbsoluteFile() can only throw a SecurityException. That's fine to keep, but you could fold both runCatching blocks into one.
  • LoggerFactory.getLogger(ToolCreatorStateHolder::class.java) is called three times in defaultParentDir. A single private val logger in the companion object (or reusing the one the class already has) would be tidier.
  • build.gradle.kts: the "Set bossconsole.release.tag in gradle.properties or pass -Pupstream.source" message appears twice, in map {} and in doLast. A local val would keep the two in sync.
  • Always wiring dependsOn("downloadApiContract") is correct. The configuration-time exists() check was wrong for clean build, and the task returns early when the jar is already there. Nice catch.

Tests

  • The 5 new tests cover the cases that matter: normal home, blank home, blocked .boss, symlinked workspaces, and project path taking precedence without creating the default. The last one asserts that .boss is not created, which is a good touch.
  • symlinked workspace root remains usable uses Files.createSymbolicLink, which fails on Windows without Developer Mode. CI runs on Linux so this is fine today. If Windows ever joins the matrix, an Assumptions-style skip on UnsupportedOperationException / FileSystemException would keep it from breaking there.
  • I counted @Test annotations and got exactly 261, matching the AGENTS.md update.

BOSS conventions

  • ✅ No Android APIs, and no Compose changes in this PR.
  • ✅ The RemotePluginContext.projectPath access is null-safe (projectPath?.let { … } ?: default).
  • ✅ No version bump in plugin.json. The release workflow owns that.
  • ✅ Both changed Kotlin files end with a newline.
  • ✅ The import reorder in the test (StateWireJson moved to the top) is a nice cleanup.

None of these block the merge. Item 1 is the one I'd fix before the next release run.

@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: fix: default OOP tool workspaces to the BOSS root

Overall this is a solid, well-scoped PR. Pinning downloadDeps to a known-compatible BossConsole tag fixes a real fragility, and the holder still publishes once from its constructor when the home is unusable, which keeps it clear of the version-0 trap described in AGENTS.md. The tests are good. The mutation check in the description, plus the "project path wins and the default isn't created" assertion, are the kind of test this repo values. Below are a few issues, roughly in priority order.

🔴 Release workflow: the new ipc_version guard runs after the version bump is pushed

The new check sits in Resolve versions (release.yml:106). The Bump runtime version in plugin.json step (line 66) runs before it, and that step commits and git pushes the bump. So a dispatch with ipc_version set and bossconsole_release_tag empty leaves main like this:

  1. plugin.json#version is bumped and pushed.
  2. The job then fails with bossconsole_release_tag is required….
  3. No release, no store publish, and a burned version number. The next attempt bumps again.

Input validation should fail before anything is mutated. I'd move the check into its own step right after Checkout (or at the top of the bump step), with the same env: indirection you used here. Using env: instead of inline ${{ inputs.* }} is a nice injection-hardening change, by the way. 👍

🟠 Tests now write into the real $HOME

ToolCreatorStateHolder(scope) now goes through initialParentDir(null) → defaultParentDir(System.getProperty("user.home")), and that mkdirs()s ~/.boss/workspaces/tools. Many existing tests construct the holder this way (ToolCreatorStateHolderTest lines 115, 123, 137, 143, 168, 276, 286, 297, and IntentDeserializerArmsTest:78). As a result:

  • Every ./gradlew test run creates ~/.boss/workspaces/tools on the developer's machine or CI runner.
  • Those tests now depend on the real home's state. The new createForHome seam shows you already saw this. For example, the Directory already exists validation would trip if a dev had a matching slug under ~/.boss/workspaces/tools.

Suggestions:

  • Point the existing tests at createForHome(scope, tempDir), or set user.home to a temp dir for the test JVM in build.gradle.kts (tasks.test { systemProperty("user.home", …) }).
  • Optionally, stop creating the directory eagerly in a constructor path and create it lazily in scaffold(). scaffold() already calls dir.mkdirs(), which creates the parents too. The cost is that the initial form would show "Location does not exist" until the first build. If you want the form valid immediately, keep the eager create, but then the test isolation above matters more.

🟡 defaultParentDir is more defensive than it needs to be

  • File.absoluteFile throws only SecurityException, and there is no SecurityManager in this child JVM (and it's deprecated). The first runCatching/early return toolsPath.path branch can't really be reached. File(userHome, ".boss/workspaces/tools").absoluteFile plus the single mkdirs() guard would read more simply.
  • mkdirs() reports a blocked or read-only path by returning false, not by throwing. The isDirectory/mkdirs check already covers the "blocked by a file" case, so the second runCatching is also mostly belt-and-braces.
  • LoggerFactory.getLogger(ToolCreatorStateHolder::class.java) appears three times. A private val log in the companion would tidy this up.

None of this is wrong; it's just more code than the failure modes need.

🟡 createForHome is a production API that exists only for tests

It's internal, so the blast radius is small. Still, initialParentDir(projectPath, userHome) already gives the seam. Another option is an internal constructor that takes parentDir, so tests don't need a factory in the companion. Not blocking.

🟡 Dev regression in copyApiJar (acknowledged in the PR body, flagging for visibility)

The old default was ~/Development/Boss/boss_plugins, so File(form.parentDir, "boss-plugin-api/build/libs") found a sibling local build of the API. With the new default that candidate never matches, so in-house developers will now bundle whatever is in ~/.boss/plugins, which may be an older released jar. That seems acceptable for end users, and Bundled X into libs/ already logs which jar was used. It may still be worth a note in the copyApiJar KDoc.

🟢 downloadDeps

  • Throwing inside compatibleReleaseTag.map {} and again in doLast (?: throw) duplicates the message. Since .map is lazy, both errors only surface at execution time anyway. Consider extracting the message into a val so the two can't drift.
  • Wiring dependsOn("downloadApiContract") unconditionally is the right call; the configuration-time exists() check was a real clean build bug. Please confirm downloadApiContract declares outputs or an onlyIf, so it shows up as UP-TO-DATE or SKIPPED rather than doing work on every local build.
  • AGENTS.md and the gradle.properties comment explain why latest is unsafe, which is helpful.

BOSS convention checks

  • ✅ No Android APIs, and no Compose code touched.
  • ✅ No PluginContext provider access added. context.projectPath is handled as nullable.
  • ✅ No version bump in plugin.json or build.gradle.kts (none needed).
  • ✅ Kotlin files end with a newline. The import reordering in the test file is a nice cleanup.
  • ✅ AGENTS.md test count updated to 261.

Summary

Before merging, I'd fix the release step ordering (it mutates main on a validation failure) and isolate the tests from the real $HOME. The rest are optional polish. Nice work on the non-fatal startup path and the pinned upstream tag.

@claude

claude Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review: default OOP tool workspaces to the BOSS root

This is a careful PR. The main fix holds up: the holder now always publishes its first state, even when the home directory is unusable. Without that, a broken ~/.boss would have stopped the plugin UI with no error anywhere, as AGENTS.md describes for version-0 holders. The mutation check in the description makes this convincing. Pinning bossconsole.release.tag instead of following latest is also the right call. Nothing here blocks merging. My notes are below.

What works

  • Startup can't fail on a bad home. A blank, blocked, read-only or symlinked home now falls back to the form's existing Location does not exist check instead of throwing in the constructor. The test unavailable boss path does not prevent initial state publication checks version > 0, which is the right thing to assert.
  • The order of the release checks is fixed. Validate release inputs runs before the version-bump step pushes to main, so a bad override can no longer leave a bumped-but-unreleased commit behind.
  • Inputs go through env: instead of being interpolated. IPC_VERSION_INPUT replaces IPC_VERSION="${{ inputs.ipc_version }}", which removes a script-injection path in the workflow.
  • downloadApiContract is always wired in. The comment correctly explains why the old existence check broke clean build. The task already returns early when the jar is present, so this costs nothing.
  • Tests no longer touch the real home. Overriding user.home for Test keeps the eager mkdirs() from writing to a developer's real ~/.boss. Without it, every other test that builds this holder would do so.
  • The symlink test cleans up in the right order. It deletes external before home, so deleteRecursively never follows the link into the external directory.

Suggestions (none blocking)

  1. Building the holder now creates a directory. defaultParentDir runs mkdirs() as soon as the holder is built, so ~/.boss/workspaces/tools appears for every user who loads the plugin, whether or not they ever scaffold a tool. That's defensible, since the form is valid immediately. If you'd rather not create anything at load time, create the directory just before scaffolding and let validation show "will be created". If you keep it as is, a line in the KDoc would help. Either way is fine.

  2. The API-jar sibling lookup no longer applies by default (ToolCreatorStateHolder.kt:480). File(form.parentDir, "boss-plugin-api/build/libs") is only useful when parentDir is something like ~/Development/Boss/boss_plugins. With the new default it will never match. The description already says this and the warning text was updated, so this is just a note: developers with a sibling checkout who used to get it picked up automatically now need SetParentDir or a jar in ~/.boss/plugins. It may be worth a line in the PR's release notes.

  3. downloadDeps has the same error message in two places. The map {} throws on a blank tag, and source.orNull ?: throw handles a missing property, both with the same text. That works, but pulling the message into a val would keep the two from drifting apart.

  4. runCatching { toolsPath.absoluteFile } is overly defensive. File.absoluteFile only throws SecurityException, which can't happen without a security manager. The first runCatching and its early return could become val tools = toolsPath.absoluteFile. The mkdirs() guard is the one that matters.

  5. The release inputs are only checked in one direction. Overriding ipc_version without a tag is rejected. Overriding the tag while keeping ipc.version from gradle.properties is still allowed. That case fails loudly anyway, because the asset names include the IPC version and the download would 404, so this is fine. A one-line comment saying the reverse case is deliberately left to the download would save the next reader from wondering.

  6. Keep the test count in AGENTS.md accurate. It's updated to 261, which matches the verification run. Just make sure it still matches if the PR is rebased onto other test changes.

Performance and security

  • Startup adds one isDirectory/mkdirs call, which is negligible.
  • The release workflow is safer than before because of the env: change.
  • The holder doesn't create or follow a symlink itself; it only respects one the user set up. That's the right stance.

BOSS plugin conventions

  • Compose Multiplatform: not applicable; there's no UI code in this diff.
  • PluginContext null-safety: context.projectPath is null-checked through initialParentDir. A null project path falls back to the default and never crashes.
  • Version source: no runtime version change in this PR. The new bossconsole.release.tag belongs in gradle.properties, next to ipc.version.
  • Trailing newline: both changed .kt files end with a newline (the diff has no "No newline at end of file" markers).

Test coverage

The five new tests cover the normal path, a blank home, a blocked .boss, a symlinked root, and the project path taking precedence. The last one checks that no default directory is created when a project path is given, which is a nice touch. One gap: nothing tests that SetParentDir still overrides the new default. It's covered indirectly by existing tests, but an explicit test would lock in the "this changes the default, not the user's choice" promise from the description.

Overall, LGTM once you've considered the notes above.

@shivanshu-risa
shivanshu-risa marked this pull request as draft October 9, 2026 16:13
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.

1 participant