feat: consolidate shareable Spaces and persistent download history

[kshivang]

Consolidated review of #1194 and #1195

This dev-targeted PR consolidates two legitimate, independent features from @Rushikeshiitb, reviewed together on current origin/dev:

• feat(workspace): make a saved Space shareable across machines #1194: make saved Spaces portable across machines, with project-relative path substitution and read-only MCP transforms.
• feat(downloads): persistent cross-session download history with MCP tools #1195: persist completed download history across sessions, expose it through MCP, and record completions from the existing browser download-finished path.

The features do not duplicate each other. Their only shared file is McpToolRegistryImpl, where both providers are registered. Both original commits are preserved separately, with Rushikesh's authorship and commit history intact; the registry conflict was resolved by retaining both registrations.

Review / overlap search

• Compared against latest origin/dev (bc9aa3b2f) and origin/main (6d00b9049); this branch is based on dev and does not modify shared dev or main.
• Searched open and closed PRs/issues for workspace/Space/download overlap. Adjacent work was checked, including feat(mcp): apply_template - let an agent materialise a workspace template for a project (the agent-side half of the Space picker's Templates section) #1136, fix(workspaces): normalize command separators before substitution and make normalization quote-aware (#1181) #1182, feat(cli): add boss workspace-diff - diff two saved Space files #1199, fix(security): confine DownloadDataProvider open and reveal to tracked downloads #1407, and fix(history): retain failed downloads and recent-page dismissals #1477; no true duplicate was found. fix(history): retain failed downloads and recent-page dismissals #1477 is separate failed-download/recent-page retention work and explicitly treats feat(downloads): persistent cross-session download history with MCP tools #1195 as adjacent.
• feat(workspace): make a saved Space shareable across machines #1194 and feat(downloads): persistent cross-session download history with MCP tools #1195 had no posted Claude or human review feedback; their GitHub CI rollups were green.

Validation

• ./gradlew :composeApp:desktopTest --tests 'ai.rever.boss.components.workspaces.WorkspacePortabilityTest' --tests 'ai.rever.boss.mcp.WorkspacePortabilityMcpToolProviderTest' --tests 'ai.rever.boss.downloads.DownloadHistoryManagerTest' --tests 'ai.rever.boss.mcp.DownloadHistoryMcpToolProviderTest'
• :composeApp:compileKotlinDesktop completed as part of the focused test run.

Both passed on current dev (9.5.22). The original PRs remain available in their history and are being closed as superseded by this ready dev-targeted consolidation.

[supabase]

This pull request has been ignored for the connected project pcnwqamqdnsadranufjv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[github-actions]

Claude diff review of 8c4d049 (no tests executed).

Review: PR #1506 @ 8c4d049 → dev

Scope: two new feature areas (Space portability + persistent download history), a new MCP provider each, one registry hook, one FluckEngine call site, and four new test files. Below, "confirmed" means the defect is visible in the diff text itself; "uncertain" means it depends on code not in the diff.

---

Confirmed defects

1. Unused import — likely lint/build break
composeApp/src/commonMain/kotlin/ai/rever/boss/mcp/DownloadHistoryMcpToolProvider.kt:5

import ai.rever.boss.plugin.api.McpToolArgs

Neither handler takes args (handler = McpToolHandler { handleList() } at ~L41, McpToolHandler { handleClear() } at ~L56); McpToolArgs is never referenced in the file. ktlint's no-unused-imports / detekt UnusedImports will flag this. (Contrast WorkspacePortabilityMcpToolProvider.kt, where the same import is used.)

2. Project-path parameterization is a naive substring replace — corrupts sibling paths
WorkspacePortability.kt, parameterize (~L98-108) and parameterizeCommand (~L116-119):

url = tab.url?.replace(projectPath, PLACEHOLDER),
filePath = tab.filePath?.replace(projectPath, PLACEHOLDER),
workingDirectory = tab.workingDirectory?.replace(projectPath, PLACEHOLDER),

With projectPath = "/Users/me/proj", a tab pointing at /Users/me/proj-old/src/A.kt (a different project that merely shares a prefix) becomes {projectPath}-old/src/A.kt, and on import resolves to /opt/other-old/src/A.kt — a silently wrong path on the importing machine. There is no separator/boundary check (projectPath + / or exact equality). Same problem inside initialCommand, where any occurrence of the prefix anywhere in the command string is rewritten, including in unrelated arguments or literal text.

Test gap: WorkspacePortabilityTest only uses paths that are exact-prefix children (/Users/me/proj/...), so this case is unexercised.

3. sizeBytes is dead in production
FluckEngine.kt:3409-3412 is the only recorder:

ai.rever.boss.downloads.DownloadHistoryManager
    .record(url, destinationPath)

record's third parameter defaults to null (DownloadHistoryManager.kt, record(...) ~L82-86), so DownloadRecord.sizeBytes will always be null in the shipped path, and the MCP list tool will never emit sizeBytes (DownloadHistoryMcpToolProvider.kt, recordJson ~L86). The field, its serialization, and the test asserting it (DownloadHistoryManagerTest, assertEquals(1234L, all[1].sizeBytes)) are only reachable from tests. Either populate it from the DownloadFinished event / File(destinationPath).length() or drop the parameter.

4. One bad record destroys the whole history, then gets overwritten
DownloadRecord.kt:14 — val id: String has no default, contradicting the file's own KDoc ("every field except [id] carries a default … hand-editable"). A hand-edit that drops id, or any truncation, makes Json.decodeFromString throw; DownloadHistoryManager.loadSync (~L56-70) catches it and sets _downloads.value = emptyList(). The next record() call then persists the empty-plus-one list over the file, permanently discarding the other 499 entries. There is no quarantine/backup of the unparseable file. For a store that is explicitly documented as hand-editable and forward-migrated, this is a data-loss path worth handling (rename to .corrupt before overwriting, or decode records leniently).

5. clear() early-returns without touching disk
DownloadHistoryManager.kt, clear (~L110-118):

val removed = _downloads.value.size
if (removed == 0) return@withLock 0

If the in-memory list is empty because the on-disk file failed to parse (case 4), downloads_history_clear reports success with removed = 0 and leaves the stale/corrupt file in place. A "clear the history" operation should be idempotent against disk, not against the in-memory mirror.

6. Persist failures are invisible to callers
persist (~L129-141) swallows every exception into a logger.warn. record/remove/clear have already mutated _downloads before persist runs, so a failed write leaves memory and disk divergent, and downloads_history_clear still returns {"success": true, "removed": N} to the agent (DownloadHistoryMcpToolProvider.handleClear ~L74-81). At minimum the MCP result should reflect write failure.

---

Likely issues / regressions (high-confidence reasoning, secondary code not inspected)

7. New suspend point before cleanup in the download-finished handler
FluckEngine.kt:3406-3414: DownloadHistoryManager.record(...) is a suspending call (it takes mutex.withLock and hops to Dispatchers.IO) inserted between updateStatus(...) and the map cleanup:

downloadManager.updateStatus(downloadId, DownloadStatus.COMPLETED)
ai.rever.boss.downloads.DownloadHistoryManager.record(url, destinationPath)
activeDownloadUrls.remove(url)
activeDownloads.remove(downloadId)

If the launching scope is cancelled (window close / engine shutdown) while suspended in record, the CancellationException propagates and activeDownloadUrls / activeDownloads are never cleaned — a leak, and potentially a "download already in progress" false positive for a later re-download of the same URL. Recording after the cleanup, or wrapping it in try/finally / NonCancellable, avoids widening that window.

Style note: the fully-qualified inline reference should be a normal import for consistency with the rest of the file.

8. toPortable can still export machine-absolute paths
toPortable (~L34-42) short-circuits when workspace.projectPath is null/blank and returns the input unchanged, documented as "already portable". That is only true of the workspace-level field; tab filePath/workingDirectory/initialCommand can still contain absolute paths from the authoring machine (Spaces built ad hoc rather than from a template are exactly this case). The test a Space with no project path is already portable and returned unchanged asserts assertSame on this branch and so encodes the assumption rather than testing it. Consider at least documenting that export is only sanitizing when projectPath is set, or scanning tabs for leaked home paths.

9. Placeholder token duplicated instead of shared
WorkspacePortability.PLACEHOLDER = "{projectPath}" (L27) is a second literal for a token the KDoc says WorkspacePlaceholders owns and resolve/raw/cmd delegate to (WorkspacePlaceholders.substituteProjectPath). If that constant ever changes, parameterize keeps emitting the old token and export/import silently stops round-tripping (no compile error, no test catches it — every test uses WorkspacePortability.PLACEHOLDER on both sides, so they'd still pass). Reference the WorkspacePlaceholders constant instead.

10. Import always shell-quotes, export matches quoted-then-raw — round trip is not quoting-neutral
parameterizeCommand (~L116-119) strips both the quoted and raw forms to a bare {projectPath}, but cmd (~L137-140) always re-substitutes with quote = true. So:

• cd /Users/me/proj/src → cd {projectPath}/src → cd '/opt/x'/src (works in sh, but is no longer byte-identical, and breaks for shells/contexts where the quote is not concatenation-safe).
• A placeholder authored inside existing quotes, e.g. echo "{projectPath}", resolves to echo "'/opt/x'" — literal quotes in the output.

The tests only cover the single canonical cd <quoted> && claude shape. Worth either quoting only when the placeholder occupies a whole word outside quotes, or documenting the normalization explicitly.

11. downloads_history_list has no limit/filter and exposes full local paths
DownloadHistoryMcpToolProvider.createListTool (~L31-44) declares an empty schema and handleList (~L60-72) dumps all up to MAX_ENTRIES = 500 records, each with absolute filePath and full url, into a single tool result. That is both a potentially large payload and a broad disclosure of local filesystem layout and browsing/download history to any connected MCP client. A limit/since parameter (and consideration of whether this should be readOnly-but-still-gated) seems warranted. Note also the asymmetry: DownloadHistoryManager.remove(id) exists and is tested but has no MCP surface — only all-or-nothing clear.

12. Blocking file I/O in a static initializer
DownloadHistoryManager.kt:52-55:

init {
    storageFile.parentFile?.mkdirs()
    loadSync()
}

mkdirs() + readText() of up to 500 records run on whichever thread first touches the object. Given the MCP handler and FluckEngine both reference it from arbitrary threads (potentially the UI thread via a tool call), a lazy/async load would be safer. Also note resetForTesting (~L72-76) does not mkdirs() the injected file's parent, so a test passing a not-yet-created directory would silently fail every persist.

13. Test-only mutable statics in production source
internal var storageFile / internal var clock (L44-50) plus resetForTesting live in commonMain. The diff states this mirrors RunConfigurationManager, so it may be the house pattern, but the two new test classes (DownloadHistoryManagerTest, DownloadHistoryMcpToolProviderTest) both mutate the same singleton and both redirect it to their own temp file. If the Gradle test task ever runs these classes concurrently in one JVM (maxParallelForks with class-level parallelism, or JUnit parallel execution), they will clobber each other's storageFile and flake. Nothing in the diff pins them to separate forks.

---

Smaller observations

• DownloadHistoryManager.record calls clock() twice (~L84 for the id, ~L89 for completedAt), so the id's timestamp and completedAt disagree. Harmless with wall-clock, but with the injected counter clock in the history is bounded… test they differ by one; capture once.
• Id uniqueness is "download-${clock()}-${(0..9999).random()}" — two downloads finishing in the same millisecond have a 1-in-10 000 collision chance, and a collision makes remove(id) delete both records. A UUID would be cheaper than reasoning about it.
• @Suppress("TooManyFunctions") on WorkspacePortabilityMcpToolProvider (L24) guards a class with 6 functions; likely unnecessary, and detekt's UnnecessarySuppression rule (if enabled) would flag it.
• WorkspacePortability.fromPortable (~L49-57) performs no validation of projectPath (absolute? existing? contains {projectPath} itself?). The MCP tool only checks isNullOrBlank (WorkspacePortabilityMcpToolProvider.handleResolvePortable ~L91-99).

---

Test gaps

1. Split-tree recursion is untested. mapTabs handles VerticalSplit and HorizontalSplit (~L62-82), but every fixture in WorkspacePortabilityTest and WorkspacePortabilityMcpToolProviderTest uses SplitConfig.SinglePanel. A regression that drops one branch of a split (e.g. copying left into right) would pass all current tests.
2. No prefix-collision test for defect 2 (/Users/me/proj vs /Users/me/proj-old).
3. No test for a Space with projectPath == null but absolute tab paths (defect 8) — the existing test asserts the no-op instead.
4. toPortable idempotence / already-portable input ({projectPath} already present in a tab) is untested in both directions.
5. record with sizeBytes is only reachable from tests (defect 3); there is no test asserting the FluckEngine integration records anything, so the sole production call site is unverified by this PR.
6. Corrupt-file behaviour is tested only for "loads empty" (a corrupt file loads as an empty history rather than throwing); the follow-on overwrite/data-loss behaviour (defect 4) is not asserted either way.
7. persist failure path is untested — no test makes the file unwritable to check that callers/MCP results behave sanely (defect 6).
8. clear() on an empty-in-memory-but-nonempty-on-disk state is untested (defect 5).

---

Uncertain — needs verification against code not in the diff

• WorkspacePortability.kt (commonMain) references CommandProcessor.quotePath and WorkspaceSerializer unqualified with no import, so both must already live in ai.rever.boss.components.workspaces and be available in commonMain. If CommandProcessor is desktop-only, this file will not compile for common.
• Similarly, DownloadHistoryManager.kt is in commonMain yet imports java.io.File and calls System.currentTimeMillis(). That is fine for a JVM-only "common", but it does hard-couple the shared source set to the JVM.
• McpToolArgs(mapOf(...), "{}") and tool.handler.call(args) in the two MCP tests assume a specific constructor/interface shape; not verifiable here.
• The claim that readOnly = false routes downloads_history_clear through an ASK approval gate (KDoc, DownloadHistoryMcpToolProvider.kt ~L18-22) depends on McpToolRegistryImpl's classification, which this diff only touches to add two registerProvider calls (McpToolRegistryImpl.kt:127-129). Worth confirming the gate actually keys off readOnly.

[fluck-boss]

This now conflicts with dev - composeApp/src/commonMain/kotlin/ai/rever/boss/mcp/McpToolRegistryImpl.kt moved on with the MCP snippet-library and download-gate merges. The shareable Spaces + persistent download history work is wanted. Please rebase onto current dev and resolve the registry clash; marking as draft meanwhile. Will re-review as soon as it is green.

[fluck-boss]

Conflicts with current dev in McpToolRegistryImpl.kt - needs a rebase before it can gate (details in comment).

[Rushikeshiitb]

The only conflict with current dev is the McpToolRegistryImpl init block: dev now registers Workspace + Snippet + Notification + Introspection providers, while this branch added WorkspacePortability + DownloadHistory when it only registered Workspace. The resolution is the union of all six:

init {
    registerProvider(WorkspaceMcpToolProvider)
    registerProvider(WorkspacePortabilityMcpToolProvider)
    registerProvider(DownloadHistoryMcpToolProvider)
    registerProvider(SnippetMcpToolProvider)
    registerProvider(NotificationMcpToolProvider)
    registerProvider(IntrospectionMcpToolProvider)
}

That is the entire clash - every other file in this PR is new (the two providers, the managers, the FluckEngine download hook, and the four test files auto-merged cleanly).

I've rebased both commits onto current dev with that resolution and verified locally: the four feature test classes pass (6 + 4 + 6 + 4 = 20 tests, 0 skipped, 0 failures) and :composeApp:ktlintCheck/detekt are green. The rebased branch is on my fork at Rushikeshiitb:rebase/1506-on-dev (both original commits, authorship preserved) if it is easier to fast-forward this PR onto it. Happy to adjust however you prefer.

[github-actions]

Claude diff review of ae06484 (no tests executed).

Review — PR #1506 @ ae06484

Scope: diff-only. I have not run the build or tests and have not inspected WorkspacePlaceholders.substituteProjectPath, CommandProcessor.quotePath, TabConfig, atomicWriteText, McpToolArgs, or the mutating-tool gate — where a finding depends on those, I say so.

---

Confirmed defects

1. parameterizePath only understands / separators and exact-match roots — Windows and trailing-slash project paths silently leak / break round trip

WorkspacePortability.kt ~110-119:

path == projectPath -> PLACEHOLDER
path.startsWith("$projectPath/") -> PLACEHOLDER + path.removePrefix(projectPath)
else -> path

• A project path of C:\Users\me\proj with a tab filePath = C:\Users\me\proj\src\A.kt matches neither branch (startsWith("C:\Users\me\proj/") is false), so the absolute machine path is exported verbatim and the "portable" Space is not portable. This is a desktop app that ships a Windows-capable browser engine, so Windows paths are plausible input.
• A projectPath with a trailing slash (/Users/me/proj/) also matches neither branch for descendants ("/Users/me/proj//"), and matches == for nothing, so everything leaks.
• No normalisation (File(...).canonicalPath-style) and no case-insensitive comparison for macOS/Windows volumes.

The KDoc at ~9-24 asserts the exported JSON is project-relative; for these inputs it is not, and the failure is silent (no error, no warning).

2. fromPortable / workspace_resolve_portable accepts a non-portable workspace and silently produces a broken Space

WorkspacePortability.kt ~48-56 and WorkspacePortabilityMcpToolProvider.kt ~86-103 validate only that the JSON parses and projectPath is non-blank. If an agent (easy mistake — both tools take a field literally named workspace) feeds a materialised Space into workspace_resolve_portable, the result is:

• projectPath overwritten with the new path,
• every tab still holding the old machine's absolute paths (no {projectPath} tokens to substitute),
• a fresh id, so it looks like a legitimately imported Space.

There is no require(workspace.projectPath == null) or "no placeholder found" diagnostic. Suggest rejecting input whose projectPath is non-null, or at minimum reporting how many placeholders were substituted.

3. Asymmetric quoting for commands where the path was quoted in a form other than quotePath's

WorkspacePortability.kt ~130-132:

val pathPattern = Regex("(?<![\\w./~-])${Regex.escape(projectPath)}(?=/|$|[\\s\\\"';&|)])")

The lookbehind excludes [\w./~-] and the lookahead includes " and ', so a raw path inside quotes is deliberately matched: cd "/Users/me/proj" → cd "{projectPath}". On import, resolve routes initialCommand through cmd(...) → substituteProjectPath(..., quote = true) (~135-156), which produces cd "'/opt/x'" if that helper quotes unconditionally — a literal directory name containing quotes. The inverse pairing claimed in the KDoc (~121-125, "the exact inverse of substituteProjectPath with quote = true") only holds for commands whose path was written exactly as quotePath writes it. Confirmed as an asymmetry in this diff; the concrete broken output depends on substituteProjectPath's quoting policy, which I could not inspect.

Related boundary incompleteness in the same regex: : is not in the lookahead set, so export PATH=/Users/me/proj:/usr/bin keeps the absolute path in the export; ,, >, <, backtick likewise.

4. loadFailed is sticky and permanently, silently disables download recording

DownloadHistoryManager.kt ~67-82 sets loadFailed = true on any parse failure; record (~95-99) then hard-fails:

check(!loadFailed) { "Download history could not be read; clear it before recording new downloads" }

The only caller, FluckEngine.recordCompletedDownload (~3504-3518), swallows it with a logger.warn. Combined with DownloadRecord.id having no default (DownloadRecord.kt:14) while the file is documented as hand-editable, a single malformed record — or one entry missing id — makes the entire history unreadable, and from then on no download is ever recorded, with no user-visible signal. The only documented recovery in this diff is the downloads_history_clear MCP tool; there is no UI affordance in the diff. Consider quarantining the bad file (rename to .corrupt) and starting fresh, or per-record lenient decoding, rather than a latch that mutes the feature indefinitely.

5. FluckEngine reorder comment does not describe what the code guarantees; stat calls run off the IO dispatcher

FluckEngine.kt ~3459-3468:

scope.launch {
    // The native download is finished. Release its reservations before the first
    // suspension so cancellation cannot strand the URL or destination path.
    activeDownloadUrls.remove(url)
    ...

The body already sits behind scope.launch, i.e. behind a dispatch. If the scope is cancelled before the coroutine starts, the reservations are stranded exactly as before — the reorder only helps when updateStatus suspends and is cancelled mid-body. The comment overstates the guarantee; if the intent is cancellation-proof cleanup, the map/releaseFilePath calls belong in the listener body (outside launch) or in a NonCancellable block.

Separately, recordCompletedDownload (~3507-3511) performs isFile + length() on whatever dispatcher scope uses (not visible in the diff); these are blocking filesystem stats. DownloadHistoryManager.persist correctly wraps its write in Dispatchers.IO (~147-156), so the inconsistency is worth fixing.

Also note a behavioural window introduced by the reorder: activeDownloads/activeDownloadUrls no longer contain the download while its status is still pre-COMPLETED. Whether anything (cancel handler, UI lookup) depends on that invariant is not verifiable from the diff.

---

Uncertain observations

• quotePath semantics gate a potentially serious bug. Line ~130 does an unbounded String.replace(CommandProcessor.quotePath(projectPath), PLACEHOLDER) with no boundary check. If quotePath returns the path unquoted when it needs no quoting (a common implementation), then for projectPath = /Users/me/proj the command cat /Users/me/proj-old/A.kt becomes cat {projectPath}-old/A.kt — corrupting an unrelated sibling path. The new test export leaves sibling paths and command arguments untouched (WorkspacePortabilityTest.kt ~70-92) would catch this only if quotePath always quotes. Please confirm quotePath's contract; if conditional, the raw-form regex must be the only matcher.
• "Safe to share" is overstated. toPortable rewrites only the Space's own project path. Any other absolute path — a tab pointing at ~/notes, a workingDirectory outside the project, a command referencing /Users/alice/.ssh — is exported verbatim, including the username. WorkspacePortabilityMcpToolProvider.kt ~28-33 tells the agent the output is "safe to share (write it to a file, commit it, send it)". The test only asserts the project path is absent (WorkspacePortabilityTest.kt ~63). Consider softening the description or scrubbing/flagging residual absolute paths.
• Import of untrusted portable JSON is arbitrary command execution. fromPortable faithfully preserves initialCommand for terminal tabs. A shared/committed portable Space is therefore executable content; nothing in this diff warns or sanitises at the import boundary. The MCP tool itself is a pure transform (correctly readOnly = true), but the surface it creates ("commit it, send it" → create_workspace/open_workspace) deserves an explicit trust note.
• Field coverage. parameterize/resolve (~98-107, ~136-145) touch only url, filePath, workingDirectory, initialCommand. If TabConfig carries any other path-bearing state (extra args, per-plugin metadata maps, session/profile paths), those leak. Not verifiable from the diff.
• Blank projectPath into fromPortable. Only the MCP layer checks blankness (WorkspacePortabilityMcpToolProvider.kt ~95-97); the public fromPortable/fromPortableJson do not, so a blank binding yields paths like /index.html.
• commonMain using JVM-only APIs. DownloadHistoryManager.kt imports java.io.File, java.util.UUID and calls System.currentTimeMillis() in commonMain. That compiles only if every target is JVM/Android. The file references BossDirectories.resolve returning a File, which suggests this is already the house pattern — flagging only in case a non-JVM target exists.
• Test-driven singleton mutation. storageFile/clock are @Volatile internal var on a global object (~38-42), mutated by two test classes (DownloadHistoryManagerTest, DownloadHistoryMcpToolProviderTest). Safe if classes run sequentially in a JVM; flaky if the Gradle test task enables parallel class execution in-JVM. An injected-dependency seam would be sturdier than a mutable singleton hook.
• DownloadHistoryManager.remove(id) (~122-134) has no production caller in this diff — no MCP tool, no UI. Either dead code or a follow-up dependency.
• No trimming on load. MAX_ENTRIES is enforced only in record (~113). A hand-edited or previously-larger file keeps >500 entries in memory until the next record.
• Registration. McpToolRegistryImpl.kt:166-167 adds both providers; I cannot confirm from the diff that workspace_make_portable / workspace_resolve_portable / downloads_history_* don't collide with names already exported by the existing workspace/browser providers, nor how the registry behaves on collision.
• Only FluckEngine records. Any other completion path (other engine, save-as flow, resumed download) won't appear in history, and a repeated DownloadFinished event would insert a duplicate (no dedupe on url+path).

---

Test gaps

Confirmed from the diff (the new tests are otherwise well-targeted):

1. file:// branch untested. parameterizePath has a dedicated file:// prefix branch (~115), but the sample browser tab uses a bare path "/Users/me/proj/index.html" (WorkspacePortabilityTest.kt ~33). No test exercises file:///Users/me/proj/x, which is the realistic browser-tab value and the only reason that branch exists.
2. mapTabs recursion untested. Every test uses SplitConfig.SinglePanel only (tabs(ws) casts to it, ~57). The VerticalSplit / HorizontalSplit arms (~77-89) — where a transposed left/right or dropped subtree would be invisible — have no coverage.
3. No Windows/trailing-slash path cases (defect 1) and no double-quoted command path case (defect 3).
4. workspace_resolve_portable given a non-portable Space (defect 2) is untested — the provider test only covers missing-arg and invalid-JSON errors.
5. handleList's loadFailed error branch (DownloadHistoryMcpToolProvider.kt ~66-69) and handleClear's persistence-failure branch (~82-89) are untested; DownloadHistoryMcpToolProviderTest only covers happy paths.
6. clear() after a failed persist (does loadFailed stay true? does state stay intact?) is untested, and load-time trimming to MAX_ENTRIES is untested.
7. DownloadHistoryManagerTest "a failed save does not claim an in-memory change" (~120-129) induces failure by replacing the target file with a directory. Whether that fails depends on atomicWriteText's rename semantics per-OS; it is a plausible source of platform-specific flake and doesn't assert which exception/that the on-disk file is unchanged.

---

Summary

The workspace-portability core is a reasonable design, but it is /-and-POSIX-specific, has no guard against resolving an already-bound Space, and its command quoting is only self-inverse for commands the template itself wrote — all three fail silently, which is the worst mode for a share/import feature. The download history's loadFailed latch converts a single corrupt byte into permanent silent feature loss. The FluckEngine reorder is harmless but its stated rationale doesn't hold, and its stat calls should move to Dispatchers.IO.

[github-actions]

Claude diff review of a74a80f (no tests executed).

Review: PR #1506 @ a74a80f → dev

Scope: workspace export/import (WorkspacePortability + MCP provider), persistent download history (DownloadHistoryManager + MCP provider), one FluckEngine hook, plus tests. Line refs are symbol-anchored (new files, so offsets are approximate). I reviewed only the diff text; I have not run the tests and cannot see CommandProcessor.quotePath, WorkspacePlaceholders.substituteProjectPath, TabConfig, atomicWriteText, or McpToolArgs.

---

Confirmed defects

1. WorkspacePortability assumes / separators — export is a silent no-op on Windows

WorkspacePortability.kt, parameterizePath:

path.startsWith("$projectPath/") -> PLACEHOLDER + path.removePrefix(projectPath)

and parameterizeCommand's lookahead (?=/|$|[\s\"';&|)]), and cmd's Regex("(?<![\"'])\\{projectPath\\}(/[^\\s\"';&|)]*)").

With projectPath = C:\Users\me\proj and a tab filePath = C:\Users\me\proj\src\A.kt, neither the equality nor the "$projectPath/" branch matches, so the path falls through to else -> path. toPortable still sets projectPath = null, so the "portable" JSON contains the origin machine's absolute paths and has lost the root needed to re-parameterize it. The class doc explicitly claims "Exported verbatim, those paths point at a directory that does not exist on anyone else's machine" — that failure mode is exactly what Windows users get. Note the code itself acknowledges Windows (cmd: "PowerShell cannot concatenate…"), so this isn't a POSIX-only module. No test uses a backslash path.

2. Unnormalized projectPath (trailing separator) produces the same silent leak

Same parameterizePath. For projectPath = "/Users/me/proj/":

• path == projectPath → false for /Users/me/proj/src/A.kt
• path.startsWith("/Users/me/proj//") → false

So nothing is parameterized, yet toPortable clears projectPath. A single trailing slash (trivially present in user-entered or File.separator-joined paths) turns a "portable" export into a leaky, unusable one. Recommend normalizing (trimEnd('/', '\\'), and platform-separator handling for #1) once at the top of toPortable/fromPortable, plus tests for both shapes.

3. workspace_make_portable succeeds on a Space it cannot make portable

WorkspacePortability.toPortable:

if (projectPath.isNullOrBlank()) return workspace

WorkspacePortabilityMcpToolProvider.handleMakePortable returns that unchanged JSON as a non-error result, while the tool description promises "Rewrite a saved Space's absolute project path … so the JSON can be shared across machines." An agent (or a user acting on the agent's output) will commit/share JSON containing absolute local paths believing it was sanitized. This should be McpToolResult(..., isError = true) or at least carry an explicit warning field. Combined with #1/#2 the same holds for those cases. No test covers toPortableJson of a projectPath-less Space (WorkspacePortabilityTest only asserts assertSame on the object identity path).

4. DownloadHistoryManager: one transient read failure disables recording for the whole process, and the only recovery destroys data

DownloadHistoryManager.loadSync sets loaded = true in finally and loadFailed = true in the catch; ensureLoaded never retries (if (loaded) return). record/remove/list then all hard-fail:

check(!loadFailed) { "Download history could not be read; clear it before recording new downloads" }

and FluckEngine.recordCompletedDownload swallows that IllegalStateException. So any read error — not just a corrupt document, but a momentary share-violation/lock (common on Windows), a permissions blip, a partially-flushed file — permanently drops every download record for the session with only a logger.warn. The documented remedy (clear()) is the one operation that overwrites the possibly-recoverable file. Suggest distinguishing parse failure from IO failure (retry the latter), and/or renaming the bad file aside (download-history.json.corrupt) instead of requiring destructive clear().

---

Security / privacy

5. downloads_history_list is an ungated read of full download URLs

DownloadHistoryMcpToolProvider.createListTool declares readOnly = true, which per the class doc means "left at ALLOW by the mutating gate" — no approval prompt. The handler returns record.url verbatim (recordJson), and FluckEngine.recordCompletedDownload records every completed browser download unconditionally, with no opt-out and a 500-entry retention.

Download URLs routinely carry secrets: pre-signed S3/GCS URLs, one-time download tokens, ?access_token=/?sig= query params, and internal hostnames. The tool description even advertises "including full URLs and local paths". Any MCP client — including an agent that has ingested prompt-injected web content in the very browser that produced these downloads — can enumerate them without a gate, plus the full local filesystem layout via filePath. Recommend at minimum: strip query/fragment (or redact) by default with an explicit opt-in to full URLs, classify the tool as ASK, and/or make the history opt-in. Note also that ~/.boss/download-history.json is written in plaintext with default permissions.

6. workspace_resolve_portable interpolates a caller-supplied projectPath into initialCommand with no validation

handleResolvePortable accepts any non-blank string and passes it to WorkspacePortability.fromPortableJson → cmd():

val quotedDescendants = descendant.replace(content) { CommandProcessor.quotePath(projectPath + match.groupValues[1]) }
return WorkspacePlaceholders.substituteProjectPath(quotedDescendants, projectPath, true)

Descendant occurrences are quoted, but the descendant regex deliberately skips placeholders preceded by "/' ((?<![\"'])), so for cd "{projectPath}/src" the whole burden of correct quoting falls on substituteProjectPath(..., quote = true). I can't see that function, so this is an observation, not a confirmed hole — but if it inserts the value raw when it detects an enclosing quote (which the test quoted placeholders resolve once implies it does), then a projectPath containing " breaks out of the quoting: /tmp/x"; rm -rf ~; echo " → cd "/tmp/x"; rm -rf ~; echo "". Cheap fix: validate at the tool boundary (absolute path, no "/'/newline/control chars) and add a unit test for a projectPath containing a quote.

Related, and worth being deliberate about: these tools make resolve_portable a laundering step for arbitrary initialCommand shell text from untrusted JSON. That's defensible given the doc's "Saving or opening the result is done through the existing workspace tools", but it means the security boundary is entirely on create_workspace/open_workspace — the diff doesn't show those gates, and nothing here re-validates the tab payload.

---

Uncertain observations / regression risk

7. FluckEngine reorder changes the observable completion sequence

activeDownloadUrls.remove(url)
activeDownloads.remove(downloadId)
FileSystemUtils.releaseFilePath(destinationPath, owner = downloadId)
downloadManager.updateStatus(downloadId, DownloadStatus.COMPLETED)

COMPLETED is now published after the tracking maps are cleared (previously before). Any collaborator that reacts to the COMPLETED transition by looking up activeDownloads[downloadId] (reveal-in-folder, retry, re-resolving the destination) now observes it absent. I can't verify from the diff whether such a consumer exists, and there is no test for this handler. Also, the new comment claims "cancellation cannot strand the URL or destination path" — the whole body is still inside scope.launch, so a scope cancelled before dispatch runs none of it; the window is narrowed, not closed.

8. Blocking filesystem stat on an unknown dispatcher

recordCompletedDownload runs File(destinationPath).takeIf { it.isFile }?.length() on whatever dispatcher scope.launch uses (not visible in the diff), then DownloadHistoryManager.record acquires the mutex on that thread before hopping to IO for the write. If scope is UI/Swing-confined, that's blocking IO plus lock contention on the UI thread. Wrapping the stat in withContext(Dispatchers.IO) would remove the question.

9. parameterizeCommand's quoted-form replace is unanchored

val quoted = command.replace(CommandProcessor.quotePath(projectPath), PLACEHOLDER)

This is a plain substring replace with no boundary check, unlike the second (regex, boundary-aware) pass. If quotePath returns the input unchanged when no quoting is needed — a very common implementation — this would rewrite cat /Users/me/proj-old/src/A.kt into cat {projectPath}-old/src/A.kt. The export leaves sibling paths and command arguments untouched test passes only if quotePath always quotes. That invariant is load-bearing and invisible here; either assert it directly or make step one boundary-aware too.

10. Redaction covers only four fields

parameterize/resolve touch url, filePath, workingDirectory, initialCommand. Any other path-bearing field on TabConfig/PanelConfig/LayoutWorkspace (env maps, args, session/profile paths) and any free-text field that commonly embeds a cwd — title (terminal tabs are frequently titled with their directory), name, description — carries the absolute origin path into the exported JSON. The strongest test assertion,

assertFalse(WorkspacePortability.toPortableJson(sampleWorkspace(projectPath)).contains(projectPath))

uses a fixture whose titles are "B"/"E"/"T", so it cannot detect this. Suggest a fixture that puts projectPath into every string field and keeping that JSON-wide assertion.

11. Nit: JVM-only APIs in commonMain

DownloadHistoryManager.kt (commonMain) imports java.io.File, java.util.UUID, and calls System.currentTimeMillis(). Fine if targets are desktop+Android (and consistent with BossDirectories.resolve returning File), but it's a new hard JVM dependency in shared code.

---

Test gaps

• No Windows-path or trailing-slash case for WorkspacePortability (defects ⬆️ actions:(deps): Bump supabase/setup-cli from 2 to 3 #1, ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2 are both invisible to the current suite).
• parameterizePath's file:// branch is never exercised — no test has a file:// URL, so that recursion (and its interaction with raw/substituteProjectPath) is unverified.
• No coverage of toPortableJson on a projectPath-less Space producing non-portable output (⬆️ actions:(deps): Bump actions/setup-go from 6 to 7 #3).
• FluckEngine.recordCompletedDownload and the reordered DownloadFinished handler are untested (docs: document Toolbox and MCP in README; fix open-source staleness #7, docs: rewrite README as a showcase (AI agents, governance, Toolbox, BossTerm sharing) #8).
• No assertion that the two new providers are registered in McpToolRegistryImpl.kt:163-167, nor that the four new tool names (downloads_history_list/_clear, workspace_make_portable/_resolve_portable) don't collide with existing host tools — a duplicate name is exactly the kind of regression a registration-list edit introduces.
• Test-harness hermeticity: both DownloadHistoryManagerTest.tearDown and DownloadHistoryMcpToolProviderTest.tearDown call DownloadHistoryManager.resetForTesting() with no file, which falls back to defaultStorageFile and, with loadNow = true, runs storageFile.parentFile?.mkdirs() + loadSync(). The suite therefore creates ~/.boss if absent and loads the developer's/CI agent's real download history into the shared StateFlow. Prefer resetForTesting(throwawayFile, loadNow = false).
• Shared-singleton locking assumption: the two download tests use org.junit.jupiter.api.parallel.ResourceLock("DownloadHistoryManager") while declaring lifecycle with kotlin.test.@BeforeTest. If this module's kotlin-test maps to JUnit4, that annotation is inert and the two classes can interleave on the same singleton (storageFile, clock, loadFailed, _downloads) under parallel execution. Worth confirming the engine in this module.