fix(cache): keep cache refusals from blocking verified installs

[Raghav-Bansal-15]

A verified cache entry could disappear between lookup and copy, failing an install instead of downloading it.

Fall back to a fresh verified download when the cache copy is unavailable. Signature enforcement remains outside the best-effort cache wrapper.

Validation at d3731ec9: 71 repository tests and ktlint/detekt pass locally. The new race regression fails with the unprotected copy and passes with the fallback.

Original and reviewer commits are preserved. Upstream workflow approval and required checks are separate gates; no merge or release.

Hosted validation: all current-head fork workflows passed at d3731ec9: 🔨 Build & Test. Upstream approvals and the stated non-CI gates remain separate.

[kshivang]

@claude review the current plugin cache identity and linked-path hardening, including callers and regression coverage.

[claude]

Claude finished @kshivang's task in 5m 15s —— View job

---

Review: plugin cache identity + linked-path hardening

• Read the diff against origin/dev
• Audit the new identity keying and linked-path checks
• Trace every caller for the new throwing contract
• Assess regression coverage and CI portability

The core idea is right and the escape surface really is closed: every path component is now a SHA-256 of the exact input, so no id/version can alias another or steer the path, every stat and traversal uses NOFOLLOW_LINKS, Files.walk never follows a link, deleteEntries deletes the link rather than its target, and cacheJar publishes through ATOMIC_MOVE so a planted entry is replaced rather than written through. The two symlink tests are genuine mutation tests - drop either check and they delete the surrounding temp tree.

What I'd want changed is the failure mode, not the checks. Three of them turn a cache anomaly into a hard install failure, and the recovery path is not reachable from the app.

---

High

1. A planted link at the entry path permanently blocks installing that plugin/version, and nothing can clear it

cacheFile throws IllegalStateException (PluginDownloadCache.kt:189) for a symlinked entry, and pluginDirectory throws for a symlinked plugin dir (:178). Both are on the path of getCachedJar, cacheJar and removeCachedJar. In the caller, downloadPlugin wraps everything in runCatching (RemotePluginRepository.kt:317), so that ISE comes back as Result.failure - the download is never attempted, and the purge API that would clean it (removeCachedJar, wired as onVerificationFailure at RemotePluginRepository.kt:349) throws from the same cacheFile call.

The only thing that clears it is clearCache(), which has no production caller - PluginStoreSetup.clearCache (:432) and getCacheStats (:439) are unreferenced anywhere in composeApp. So the recovery is "delete ~/.boss/plugin-cache by hand".

Worth noting the throw isn't what provides the safety here. With the check at :189 removed, getCachedJar is still safe, because Files.isRegularFile(path, NOFOLLOW_LINKS) is already false for a link (:38), and cacheJar is still safe, because Files.move(..., REPLACE_EXISTING) unlinks the symlink rather than writing through it. A cache's correct answer to a corrupt entry is "miss, and remove it" - same shape as the existing SHA-mismatch branch at :52-56. I'd delete the link and return null in getCachedJar, and let cacheJar proceed into the atomic move.

Fix this →

2. A cache-write failure now fails an already-verified download

downloadCache.cacheJar(...) is called at RemotePluginRepository.kt:438 - after the SHA-256 check, after enforceStoreSignature, and after PluginSignatureSidecar.persist. The jar is complete and trusted at targetPath. Any throw from cacheJar (the new require, either check, or an IOException from createTempFile/move on a full or read-only disk) now discards that work and reports the install as failed. Caching is an optimisation; it should not be able to fail the operation it was optimising. Wrap the call site in its own runCatching and log.

3. The new require on id/version syntax can make a plugin uninstallable, and it isn't load-bearing

cacheKey (:196) rejects anything outside [A-Za-z0-9._+-], longer than 4096 chars, or ending in .. The old code sanitized instead (pluginId.replace(Regex("[^a-zA-Z0-9._-]"), "_")), so those inputs worked. Now a single store row with, say, 1.0.0 (beta), 1.0~rc1, a non-ASCII version, or an empty version string makes getCachedJar throw before the download is ever attempted - downloadPlugin fails outright rather than falling back to a fetch. Nothing between the store row and this call validates either field.

Since the emitted filename is v-<64 hex>.jar / p-<64 hex>, no input can affect path syntax at all - the charset rule, the trailing-dot rule and the 4096 bound are guarding a filename that is already a hash. I'd keep only "non-blank" (hashing "" is meaningless) plus a generous length bound, and drop the charset check. If the project does want a syntax contract on store ids/versions, it belongs where the row is parsed, with an error that says so - not as an IllegalArgumentException from a cache.

The bound-on-metadata-reads justification in the comment at :195 doesn't quite hold either: readMetadata already caps its own read at 16 KiB independently (:140-142).

---

Medium

4. All BossLogger use was deleted - AGENTS.md mandates BossLogger for this repo, and the removed WARN on Cached JAR SHA-256 mismatch, removing was the only signal that a cache entry had been tampered with. It now deletes silently (:52-56). Same for clearCache/removeAllVersions/cleanOldEntries, and readMetadata's runCatching { ... }.getOrNull() (:135-145) swallows a malformed sidecar, an I/O error and a "cache directory was replaced" check with no trace. For a change whose whole purpose is hardening, I'd restore at least the mismatch, the refusal and the metadata-unreadable paths. The class KDoc and per-method KDoc also went with it, which is a separate readability loss on a public API.

5. rootIdentity is vacuous on Windows - BasicFileAttributes.fileKey() returns null on the Windows provider, so rootIdentity is null at construction and the check at :163 compares null == null and always passes. "Plugin cache directory was replaced" is therefore a POSIX-only guarantee. That's acceptable, but it should say so in a comment, and the KDoc shouldn't imply otherwise. There's also no test for this branch on any platform.

6. Legacy entries are, in practice, never reclaimed - and they skew the stats - the comment at :205 says legacy entries "remain removable by clear/expiry", but clearCache has no production caller and cleanOldEntries has none at all (only its own definition and the new test). removeAllVersions can't see them, since it resolves p-<hash>. So every existing install keeps its full pre-change cache on disk forever. Meanwhile getCachedFileCount (:117) does count legacy jars via entries(), while getCachedPluginCount (:114) goes through listCachedPlugins and needs a sidecar - so getCacheStats can report fileCount: 12, pluginCount: 0 for the same directory. A one-time legacy sweep at construction (or a cleanOldEntries call during store init) would close both.

7. The constructor does I/O that can throw, and silently accepts a linked root - Files.createDirectories(cacheDir.toPath()).toRealPath() (:22) runs in the initializer, and RemotePluginRepository takes PluginDownloadCache() as a default argument (RemotePluginRepository.kt:38), so an unwritable ~/.boss now fails construction of the repository rather than degrading. It's caught by PluginStoreSetup.initialize's try/catch, so nothing crashes - but the store ends up with no remote repository at all where it previously had one with a dud cache. Separately, toRealPath() follows a symlinked cache root, so a linked root is silently accepted while a linked subdirectory is refused. That asymmetry is defensible (the root is host-supplied) but deserves one line saying it's intended.

---

Low / nits

• :196 compiles a fresh Regex on every call; hoist to a private val (or a companion const-style top-level) if the check survives point 3.
• Return-count semantics now disagree: deleteEntries counts jars only (:217), while cleanOldEntries counts every expired regular file (:154-155) - two per cached version now that there's a sidecar, so its number is roughly double the old one for the same work.
• cleanOldEntries no longer prunes the emptied p-<hash> directories; the old walkBottomUp pass did.
• A SHA-mismatch delete (:55) leaves the .json sidecar orphaned; removeCachedJar removes both, so the two purge paths differ. The orphan then counts toward getCacheSize.
• readMetadata bounds by readNBytes(16_385) (bytes) but compares text.length (chars) at :142 - harmless, just not the same quantity.
• readMetadata calls cacheFile per entry (:144), which re-runs checkRoot() + toRealPath() + readAttributes; listCachedPlugins is therefore O(n) syscalls in the root check alone. Fine at current cache sizes.
• @Synchronized is per-instance. Correct today - PluginStoreSetup:266 builds the single instance and hands it to RemotePluginRepository at :343 - but worth a KDoc note that it is not a cross-process lock (the atomic moves carry that weight) so a future second instance isn't assumed to be serialized.
• paths.filter { it != directory || includeRoot } (:216) reads oddly when includeRoot is true (the predicate is a constant); if (includeRoot) paths else paths.filter { it != directory } says it plainly.

---

Regression coverage

What's there is well chosen - the collision/case test, the long-id test, and both symlink tests all fail if you revert the thing they cover. Gaps:

1. Windows portability of the two symlink tests. ./gradlew build on windows-latest (build.yml:242) runs check → desktopTest, so cleanupDoesNotTraverseLinkedDirectories and refusesSymlinkedEntryAndPluginDirectory execute there. Files.createSymbolicLink needs SeCreateSymbolicLinkPrivilege; GitHub's Windows runners generally have it, but an Assumptions.assumeTrue guard around the link creation is cheap insurance against a red matrix leg on a check that is POSIX-shaped anyway. Also note a Windows junction is not an isSymbolicLink, so the :189 check wouldn't see one (the toRealPath() comparison in checkDirectory would) - untested either way.
2. No caller-level test. The interaction that actually regresses is downloadPlugin meeting a poisoned entry. RemotePluginRepositoryDownloadTest already exercises both the cache-hit and fresh-download wiring, so a case asserting that a planted link still ends in a successful download belongs there.
3. "Legacy cache entries can be downloaded again" is unverified. Write an old-layout file (<id>/<id>_<ver>.jar) and assert getCachedJar returns null, and that clearCache/cleanOldEntries reclaim it. That claim is in the PR description and nothing pins it.
4. No test for the metadata-loss path - delete a sidecar and assert the jar is still served by getCachedJar but absent from listCachedPlugins (i.e. that the desync at point 6 is the intended behaviour, not an accident).
5. No test for checkRoot's replaced-directory branch (and per point 5 it can't pass meaningfully on Windows).
6. rejectsPathSyntaxWithoutTouchingOutsideFiles pins the syntax require in place; if point 3 is taken, that test narrows to blank/oversized inputs.

---

Summary

The security property holds. My blocking concerns are the three High items, all the same shape: hardening that fails closed on the operation rather than closed on the cache. A cache that refuses to serve a suspicious entry is right; a cache that refuses to let the plugin be downloaded at all - and can't be cleared from inside the app - converts an attempted poisoning into a durable denial of install. Then restore the BossLogger calls, since AGENTS.md requires them and the mismatch warning is the tamper signal.
· branch fix/security-plugin-cache

[kshivang]

@Raghav-Bansal-15 review handoff after a bounded repair pass. Original authorship and c17c0ac are preserved; fixes are pushed through 0302635 on this PR, based on current dev e1926cc.

The original cache hash/path hardening is useful. I fixed the missing identity-metadata check in lookup (09d42ed). In response to the Claude review, dc89ead/030263583 isolate cache lookup/write/purge failures from the verified download result, preserve cancellation, contain cache initialization failures, restore safety warnings, and remove orphan metadata on SHA mismatch. This addresses the three High findings without relaxing the linked-path checks or the store hash/signature checks.

Not ready: final-head execution and review verification are outstanding. Five agent-added regressions cover mismatched/corrupt/missing/linked metadata, linked cache entries, unsupported version syntax, failed promotion into a nonempty directory, and an unusable cache root. They have only been inspected, not executed. The local JDK 17 validation invocation waited behind the shared build lock and was canceled before Gradle started; no test, ktlint, or detekt success is claimed. git diff --check passes. Please run:

python3 /Users/kshivang/Development/Boss/.pr-review-sessions/with-build-lock.py ./gradlew :plugin-platform:plugin-repository:desktopTest :plugin-platform:plugin-repository:ktlintCheck :plugin-platform:plugin-repository:detekt

Outside this shared review workspace, run the equivalent Gradle tasks directly. Confirm all original cache tests and download signature tests still pass, and validate the symlink cases on Windows/macOS/Linux. Then verify the High findings against the final head. Claude's completed review was on the original implementation, not these repairs; no repeated review request was sent.

Infrastructure versus author findings: the shared local build queue and fork CI approval are maintainer/infrastructure blockers, not demonstrated author test failures. Hosted runs on the preceding repair heads (34736545023 and 34736721522) are action_required. A maintainer must approve the final-head build; do not transfer old-head results. The original author defects were cache anomalies aborting installs and absent metadata lookup enforcement, addressed above but still needing execution proof.

Known follow-up from the Medium feedback: legacy files are not automatically swept and file/plugin statistics can differ until explicit cleanup. Automatic deletion at initialization was not added during this repair pass. Root links are intentionally canonicalized; fileKey replacement detection is provider-dependent, now documented. No live app validation was performed. In a disposable test profile, verify a repeat plugin install, a linked/blocked cache path falling back to a verified download with a warning, and bad signatures still being rejected.

Converting to draft under the review-handoff policy until this validation is completed. No merge or auto-merge was performed.

[kshivang]

@Raghav-Bansal-15 final disposition after the maintainer authorized suitable PRs to merge into dev: this PR remains draft, not merged.

Your exact-identity hashing, atomic cache publication and linked-path tests address a useful cache collision/path boundary. That contribution and your original commit remain credited. The three High findings in the latest Claude review have bounded repairs pushed through 0302635, but execution proof remains missing.

I refreshed dev/main and rechecked the current head. Final-head Build & Test is still action_required with zero executed jobs. The shared local build lock is still occupied. These are infrastructure/maintainer gates, not evidence of failing author tests; no required checks were bypassed.

Next steps: have a maintainer approve the final-head workflow; run the plugin-repository desktopTest, ktlintCheck and detekt tasks listed in the prior handoff; verify the new metadata and caller-fallback regressions plus existing hash/signature rejection tests across supported platforms. Final-head review verification is still needed because the latest Claude review predates the repairs.

Review choices: retained cache syntax/link refusals but isolated them at the caller, so a refused cache no longer blocks a verified download. Rejected the suggestion to serve a JAR with missing metadata because that contradicts the PR's identity-metadata guarantee; a regression now requires a cache miss. Did not preemptively skip Windows symlink tests without an actual privilege failure. Automatic legacy deletion remains a documented follow-up, rather than introducing new startup deletion during the bounded repair pass.

No new repair/review loop started. Draft status and the detailed validation handoff remain in place until these gates are met.

[Raghav-Bansal-15]

Review follow-up: Key entries by their exact identity, require matching metadata, and keep cache failures separate from download/signature verification. Retain linked-path refusals without making a cache failure an installation failure.

Validation: 70 repository tests and static analysis pass locally. Latest-head full CI is pending.

The PR is ready for review and the existing author/reviewer commits are retained. Upstream CI needs maintainer approval. I am following the latest-head checks and have not merged or released anything.

[kshivang]

@Raghav-Bansal-15 your exact hashed identity paths and metadata validation address the cache collision boundary, and the follow-up keeps lookup/write/purge refusals separate from verified downloads. I independently checked head bed5eeb3f31a3fb41a3bd4de28459d8209797423: the explicit catches cover the cache's syntax/path/I/O refusals, cancellation is rethrown first, root initialization failure is retained without preventing repository construction, and hash/signature verification stays outside the best-effort cache wrapper. The earlier High findings are addressed in this control flow; I am not recycling them.

The remaining merge gate is execution evidence on this head. I approved the fork Build & Test run, which is now queued/running rather than action_required. Your report of 70 local tests and static analysis passing is useful, but I have not independently reproduced that local run. Pending hosted validation is a maintainer/infrastructure condition, not an author defect or a failed test. No checks will be bypassed.

Please retain final-head results for :plugin-platform:plugin-repository:desktopTest, :plugin-platform:plugin-repository:ktlintCheck, and :plugin-platform:plugin-repository:detekt, including cache metadata/link refusals, verified-download fallback, and invalid-signature rejection across the supported platforms. The PR also adds push triggers for fix/security-* and unrelated fix/filesystem-service-*; please drop this review-branch scaffolding or explain why a permanent shared-CI change belongs in this cache fix. Existing legacy-cache/statistics and provider-dependent fileKey limits remain documented follow-ups, not newly claimed blockers.

Under the requested bounded-review policy I am returning this to draft pending final-head validation/review completion, without starting another repair or polling loop. Cache placement in the host's shared plugin repository is correct; #563 is a distinct wizard install-artifact lifecycle fix. No live cache/profile or production data was touched. Original contributor and prior agent repairs remain separately credited; no new source changes in this pass. @claude please review the current explicit-exception revision once, checking cache fallback and signature rejection; the previous completed review predates these repairs.

[claude]

Claude finished @kshivang's task in 4m 28s —— View job

---

Review: explicit-exception revision (head bed5eeb)

• Read the diff for the repair commits (b34f6a8 → bed5eeb)
• Verify cache fallback: which refusals are absorbed, and where
• Verify signature rejection is outside the best-effort wrapper
• Check the new caller-level regressions
• Attempt local execution (not permitted in this environment - see bottom)

Scope as requested: one pass over the current control flow, cache fallback and signature rejection. I am not re-raising the three earlier High findings - they are closed, and I traced each one rather than taking the summary.

---

What I confirmed

Cache fallback covers exactly the families the cache can raise. cacheOrNull (RemotePluginRepository.kt:54-70) catches IOException (createDirectories/move/copy/channel reads), IllegalStateException (checkDirectory, checkRoot, the entry symlink check), IllegalArgumentException (cacheKey's require) and SecurityException. I walked every throw site in PluginDownloadCache and found none outside that set. The three former Highs are each defused at the caller: a linked entry, an uncacheable version string and a failed promotion all become a warn and a fresh download.

One thing there is load-bearing and looks cosmetic. Cancellation is rethrown first (:60-61), and it has to be: kotlinx.coroutines.CancellationException is java.util.concurrent.CancellationException, which extends IllegalStateException. Move that clause below :64 and a cancelled install silently becomes "cache unavailable" plus a completed download. Worth one comment line, because the next person tidying catch order has no way to know.

Verification stays outside the wrapper. enforceStoreSignature is called at :377 (cache hit) and :459 (fresh) with nothing wrapped around it, and DownloadException extends Exception (PluginRepository.kt:128), so it cannot be swallowed by any of the four catches even indirectly. At :384 the wrapper encloses only removeCachedJar, so a purge that itself refuses cannot mask the rejection - the throw still happens. cacheJar is reached only after the SHA check, enforceStoreSignature and PluginSignatureSidecar.persist, so :474 can no longer fail a trusted artifact.

Rejection coverage is intact: invalid signature throws and deletes all files, garbage signature throws, substituted artifact…fails, store-reported version differing…fails, plus cache hit still verifies signature and purges poisoned entry and fresh download with invalid signature fails and is never cached. The four new caller-level tests are each genuine mutation tests - unwrap any one cacheOrNull and the matching test fails.

Root init is retained, not thrown (PluginDownloadCache.kt:28-34), construction cannot fail, and unavailable cache does not prevent repository construction or verified download pins it. The Windows fileKey()-is-null limit is now documented at :178.

---

Medium

1. The tamper warning now fires on every ordinary cache miss - PluginDownloadCache.kt:49-52

if (!Files.isRegularFile(path, NOFOLLOW_LINKS) || readMetadata(path) == null) {
    logger.warn(LogCategory.SYSTEM, "Ignoring plugin cache entry with invalid identity metadata")
    return null
}

A cold cache, a first-ever install of a plugin, and a hand-planted sidecar all emit the same WARN, and the text names an anomaly in the case where there is simply no file. This is the warning the previous review asked to restore as the tamper signal, and routine emission is what removes its value: an operator on a fresh profile sees it for every plugin they install. The condition also conflates two different facts. Splitting is cheap - return null silently when !Files.isRegularFile(...), and warn only when the jar exists and its metadata is absent or does not match.

Fix this →

2. cacheUnavailable drops the exception message - RemotePluginRepository.kt:76-79

It logs operation and failureType only. The refusals it now absorbs are precisely the security-relevant ones, and they are indistinguishable in the log from a full disk:

Underlying	Message	Reported as
root replaced under us	Plugin cache directory was replaced	IllegalStateException
planted link at the entry	Plugin cache entry must not be a symbolic link	IllegalStateException
linked plugin directory	Plugin cache directory must not be a symbolic link	IllegalStateException
uncacheable id/version	Invalid plugin cache ID or version	IllegalArgumentException
disk full / read-only	NIO text	IOException

All four non-IO messages are constant strings with no user data, so including failure.message for the check/require families costs nothing and is the difference between "the cache was tampered with" and "the disk is full". For IOException the NIO message embeds absolute paths (so, usernames), which is why I would not log it raw there - omit it or route it through the sanitizer, per AGENTS.md.

Related: the IllegalArgumentException case is permanent, not "unavailable". A store row with 1.0.0 (beta) will re-log on every install of that version for ever, while the word "unavailable" implies a fault that clears.

---

Low / follow-ups

• A refused or metadata-mismatched entry never self-heals. getCachedJar deletes only on SHA mismatch (:69-70); a linked entry, a mismatched sidecar and a missing sidecar all leave the file in place, and nothing in production reclaims it - PluginStoreSetup.clearCache (:434) and getCacheStats (:441) have no callers, cleanOldEntries has none at all. The consequence is now perf-only (that plugin/version misses for ever) rather than the install block, so this is correctly a follow-up - but it does mean the poison this PR detects stays on disk indefinitely, which is worth saying in the follow-up note alongside the legacy sweep.
• PluginStoreSetup.clearCache/getCacheStats call the cache unwrapped. Every remaining public cache method can now throw IllegalStateException from checkRoot or rethrow the retained init failure. Latent only because nothing calls them; a future "Clear cache" button needs the same treatment downloadPlugin got.
• Residual TOCTOU on the hit path, unchanged. getCachedJar hashes the file and returns a path; the caller copies it at :387. Under the same local-write threat model the symlink checks address, the bytes can change in between. The NOFOLLOW read and ATOMIC_MOVE publication narrow it and load-time sidecar verification is what actually covers it - just worth not claiming more than that.
• cacheKey compiles a fresh Regex per call (:212); hoist to a private val.
• java.io.IOException is fully qualified inline at :62; import it like the rest.
• cleanOldEntries counts sidecars as removed entries, so its return is roughly double the version count, and it no longer prunes emptied p-<hash> directories.
• paths.filter { it != directory || includeRoot } (:232) is a constant predicate when includeRoot is true; if (includeRoot) paths else paths.filter { it != directory } reads plainly.
• @Synchronized is per instance and not cross-process (the atomic moves carry that weight). One KDoc line would stop a second instance being assumed serialized, since PluginStoreSetup:268 is the only thing keeping it single today.
• The Windows-junction gap from the earlier review is closed in effect, though untested: a junction at the p-<hash> directory fails checkDirectory's toRealPath() comparison, and a junction cannot exist at the jar path (junctions are directories), where a directory fails the move with an IOException that is now absorbed.

Coverage

rejectsPathSyntaxWithoutTouchingOutsideFiles now pins the syntax require at the cache boundary only, which is the right place given the caller absorbs it. unsupported cache version syntax still downloads and verifies covers the download; nothing asserts the second install of such a version also downloads (i.e. that no entry was written). Still open from the earlier list, unchanged and reasonable to defer: a legacy-layout entry test, a checkRoot replaced-directory test, and no Assumptions guard on the symlink tests. On that last one - declining the skip absent a real privilege failure is defensible, but note this revision takes the symlink-creating surface on the windows-latest build leg from two tests to three.

The workflow change

I agree with dropping it. fix/filesystem-service-* is unrelated to a plugin-cache fix, and a push trigger on a branch that also has an open PR doubles the runs for everyone on those prefixes. It is a permanent shared-CI change arriving inside a targeted fix, which is the kind of thing that outlives the review it was added for. I cannot edit .github/workflows myself (GitHub App permissions do not allow workflow modifications), so that one is for @Raghav-Bansal-15.

Verdict

The cache-fallback control flow and the signature-rejection boundary are both correct on this head, and the reviewer's placement decisions (keep the refusals in the cache, absorb them at the caller, keep hash/signature outside the wrapper) hold up against the code. Nothing here is a blocker: M1 is the one I would fix before merge, because it undoes the observability the earlier review asked for, and M2 is a two-line change in the same area.

I could not run Gradle in this environment (./gradlew is not a permitted command here), so I am claiming no desktopTest/ktlintCheck/detekt result of my own - the hosted final-head run remains the execution evidence.
· branch fix/security-plugin-cache

[kshivang]

@claude please review final cache head 5866e9a once. Current dev is integrated; only four cache production/test files differ from dev. Prior High findings remain repaired. The author fixed ordinary-miss warning noise and documented cancellation catch ordering, and removed unrelated workflow triggers. Raw exception messages remain intentionally omitted because cache IO/security exceptions may carry sensitive paths; operation and exception type remain available. Please check remaining correctness and regression concerns without repeating resolved findings.

[claude]

Claude finished @kshivang's task in 4m 25s —— View job

---

Review: final cache head 5866e9a

• Read the diff against origin/dev (4 files, no .github/ delta - the branch triggers are gone)
• Verify the two prior Mediums as fixed
• Audit remaining correctness on this head
• Assess regression coverage

Scope as asked: one pass, no re-raising of the three earlier Highs or of M1/M2. I traced each rather than taking the summary.

---

Confirmed on this head

• M1 (miss noise) is fixed correctly, and the guard is tighter than it looks: Files.exists(path, NOFOLLOW_LINKS) (PluginDownloadCache.kt:49-52) can only be true here for a present non-regular entry or a present jar with absent/mismatched metadata, because a symlink at that path has already thrown out of cacheFile. A cold cache and a first-ever install are silent.
• Cancellation ordering is documented at the catch (RemotePluginRepository.kt:58-60). That comment is load-bearing - kotlinx.coroutines.CancellationException is java.util.concurrent.CancellationException, which extends IllegalStateException, so moving that clause below the IllegalStateException catch silently turns a cancelled install into "cache unavailable" plus a completed download.
• The catch set matches the throw sites of the three wrapped operations. I walked getCachedJar / cacheJar / removeCachedJar and every reachable throw is IOException, IllegalStateException (the two checks + checkRoot), IllegalArgumentException (cacheKey's require, and InvalidPathException from the retained init failure, which is an IllegalArgumentException) or SecurityException.
• Verification stays outside the wrapper. enforceStoreSignature is unwrapped at :376 and :459; only onVerificationFailure's purge is wrapped, so a refusing purge cannot mask a rejection. cacheJar is reached only after the SHA check, enforceStoreSignature and PluginSignatureSidecar.persist.
• Omitting exception messages is a reasonable call given IOException text embeds absolute paths; I'm not re-litigating it. One narrow consequence is in Low below.

---

Medium

1. The cache-hit path's copy is outside the fallback, so a cache entry that disappears between lookup and copy still fails the install

val cachedFile = cacheOrNull("lookup") { downloadCache.getCachedJar(...) }   // :360
if (cachedFile != null) {
    enforceStoreSignature(...)                                               // :376
    cachedFile.copyTo(File(targetPath), overwrite = true)                    // :387  <- unwrapped
    PluginSignatureSidecar.persist(targetPath, downloadInfo.signature)
    return@runCatching targetPath
}

copyTo throws kotlin.io.NoSuchFileException (an IOException) if the entry is gone, and it lands in downloadPlugin's outer runCatching (:350) as Result.failure - the fresh download below is never attempted. The @Synchronized lock does not cover this: it is released when getCachedJar returns, and the entry can be removed in between by removeAllVersions, clearCache, cleanOldEntries, or a second process sharing ~/.boss/plugin-cache.

Race-only, so not a blocker - but it is the one place where the stated invariant at :53 ("Cache availability must never decide whether a verified download succeeds") does not hold, and the residual TOCTOU window on this path is now the only reason the cache can still fail an install. Letting an IOException from the copy log and fall through to the fresh-download path closes it, and costs nothing on the happy path.

Fix this →

2. cacheOrNull does not catch UncheckedIOException, and every Files.walk-based method can raise it

Files.walk(...).toList() throws UncheckedIOException (a RuntimeException) when directory iteration fails mid-walk - unreadable subdirectory, a directory removed under the walk. That covers entries() and deleteEntries(), hence clearCache, removeAllVersions, cleanOldEntries, getCacheSize, getCachedFileCount and listCachedPlugins.

None of those is on the downloadPlugin path today, so this is latent rather than live. What makes it worth recording: PluginStoreSetup.clearCache (:441) and getCacheStats (:448) call straight into them with no wrapper, and on this head every public cache method can additionally throw IllegalStateException from checkRoot() or rethrow the retained init failure from location.getOrThrow(). They have no production callers, so nothing breaks now - but the first "Clear cache" / cache-stats UI wired to them inherits exactly the failure mode downloadPlugin was just repaired for. Either widen cacheOrNull (UncheckedIOException, DirectoryIteratorException) and reuse it there, or note on those two methods that a caller must handle refusals.

---

Low

• The metadata-first publication rationale doesn't hold, and its one real effect is an orphan sidecar. cacheJar:88-91 publishes the sidecar before the jar "so a failed metadata write cannot replace an existing artifact" - but CacheMetadata is a pure function of the key (the same {pluginId, version} on every write to that path), so either order leaves a consistent pair. The asymmetry it does introduce: when the jar move fails after the sidecar move succeeded, a v-<hash>.json with no jar is left behind, counted by getCacheSize and reclaimable only by expiry. Jar-first would leave nothing.
• A refused entry never self-heals, and the promotion-failure state re-warns forever. getCachedJar deletes only on SHA mismatch (:70-72); a linked entry, a mismatched sidecar, a missing sidecar and a directory at the jar path all stay. The last one is concrete - cache promotion failure preserves a successful verified download leaves exactly that state, and from then on every install of that plugin/version logs "Ignoring plugin cache entry with invalid identity metadata" and re-downloads. Consequence is perf plus log noise, not an install block, so a follow-up is right; worth adding to the legacy-sweep note that the poison this PR detects also stays on disk.
• "Identity metadata" bounds layout, not tampering. The sidecar's content is derivable from the key, so anyone who can plant a jar in the cache can plant its sidecar; what the check genuinely rules out is a stale, legacy-layout or colliding entry being served under a key it was not written for. The cacheJar comment already says "it does not vouch for bytes" - the PR description's "require matching metadata" reads stronger than that, and the actual byte binding is the store SHA-256 plus enforceStoreSignature. Worth one sentence in the description so the guarantee isn't over-read later.
• "unavailable" is the wrong word for the IllegalArgumentException case. A store row with 1.0.0 (beta) is permanently uncacheable, so that warning repeats on every install of that version while the wording implies a transient fault. A static reason discriminator ("reason" to "unsupported-key" / "refused-path" / "io"), chosen at the catch rather than taken from the message, separates "the disk is full" from "the cache refused this entry" without printing any path - it doesn't reopen the omitted-message decision.
• readMetadata's runCatching { ... }.getOrNull() (:150-162) also swallows checkRoot()'s "Plugin cache directory was replaced" (via the nested cacheFile) and, being runCatching, any Error. It fails safe as a miss, but that one condition is a refusal everywhere else in the class.
• Earlier nits unchanged and still fine to defer: per-call Regex in cacheKey (:212), inline java.io.IOException at RemotePluginRepository.kt:62, paths.filter { it != directory || includeRoot } as a constant predicate, cleanOldEntries counting sidecars (so roughly double the version count) and no longer pruning emptied p-<hash> directories.

---

Regression coverage

The four new caller-level tests are genuine mutation tests - unwrapping any single cacheOrNull fails the matching one - and lookupRequiresMatchingRegularIdentityMetadata covers all five metadata states (wrong id, wrong version, malformed, absent, linked) plus the sentinel. refusesSymlinkedEntryAndPluginDirectory and cleanupDoesNotTraverseLinkedDirectories still fail if either check is dropped.

Remaining gaps, in order of how much I'd want them:

1. Nothing asserts a second install of an unsupported-syntax version also downloads. unsupported cache version syntax still downloads and verifies proves the first one falls back; the claim that matters operationally is that no entry was written and the next install behaves identically. One extra downloadPlugin call plus assertEquals(0, cache.getCachedFileCount()) pins it.
2. preservesIdsVersionsAndCaseWithoutCacheCollisions couples two assertions invisibly. assertEquals(3, cache.clearCache()) is only 3 (not 4) because the preceding assertNull(getCachedJar(..., "invalid")) deletes the mismatched entry as a side effect. Correct, and a good implicit check of that branch - but a reader changing either line gets a confusing failure. One comment, or split the mismatch case out.
3. Still open and reasonable to defer: a legacy-layout entry test (<id>/<id>_<ver>.jar → miss, and reclaimed by clearCache), and a checkRoot replaced-directory test (POSIX-only, as :178 now documents).
4. Four symlink-creating tests now run on the windows-latest build leg (three here plus linked cache entry does not block a verified download). Declining an Assumptions guard absent a real privilege failure is defensible - just noting the surface grew, and that the hosted final-head run is the first thing that would tell you.

---

Verdict

No blockers. The cache-fallback control flow and the signature boundary are correct on this head, the two repaired Mediums are genuinely repaired, and the workflow scaffolding is gone. Medium 1 is the only remaining place the "cache never fails a verified download" invariant leaks, and it is a narrow race; Medium 2 is latent until someone wires a cache UI. Everything else is follow-up or wording.

I could not run Gradle here (./gradlew is not permitted in this environment), so I'm claiming no desktopTest / ktlintCheck / detekt result of my own - the hosted final-head run remains the execution evidence.
· branch fix/security-plugin-cache

[kshivang]

@Raghav-Bansal-15 your exact-identity cache hardening and follow-up fixes are preserved. I integrated current rebased dev, resolved the documentation conflict using current dev, and pushed 5866e9a. The resulting diff contains only four cache production/test files.

Independent final-head validation passed under the shared build lock: 70 repository tests (zero failures/errors/skips), repository ktlintCheck and detekt. No new source repair was needed. Prior metadata/fallback agent repairs remain separately credited in the description.

I approved upstream run https://github.com/risa-labs-inc/BossConsole/actions/runs/34959436380 and requested one current Claude review. Both are still running at this checkpoint; the PR stays ready, not draft. Pending CI is not an author defect, and the earlier successful fork run is not a substitute for final-head required checks. Merge into dev awaits those gates. No merge into main is authorized.

[Raghav-Bansal-15]

Follow-up at d3731ec9: The cache-copy race is fixed: a disappeared cached JAR falls back to a fresh verified download, with signature enforcement still outside the best-effort cache wrapper. The regression fails with the unprotected copy. All 71 repository tests and quality checks pass locally.

Original and reviewer commits are preserved. Remaining current-head hosted checks and upstream approval are separate gates; no merge or release.

[kshivang]

Reviewed author follow-up d3731ec against the completed Claude review: the cache-copy refusal now falls through to fresh download, while signature verification and sidecar publication remain outside the fallback wrapper. The regression deletes the actual cached file at the copy boundary and asserts downloaded bytes plus signature. This addresses Medium 1. Medium 2 concerns currently uncalled cache maintenance/statistics APIs and remains a documented follow-up; no broad exception rewrite is needed on the install path. Prior rating and original/reviewer attribution remain unchanged. Exact-head fork CI is green; upstream run 34978513594 is approved and executing. Current dev integration is conflict-free and focused validation is running.