fix(chromium): keep existing engine until new one is verified (fixes #910)

[choksi2212]

Reopened #910 - this was closed without a fix PR.

Two integrity defects in ChromiumAutoDownloader.installFromCandidates:

1. Destructive pre-delete. The non-staged install path deleted the existing engine directory before extracting, so a failed extraction left the host with neither engine. Extracted to a sibling swap directory instead, verified the new engine, then atomic-swapped via a backup dir; the previous engine is restored from the backup on any rename failure.

2. macOS ditto fallback. On macOS, when ditto failed the installer fell back to extractWithJava, which materialises symlink entries as text files. A Chromium Framework.framework/Versions/Current that should be a symlink became a literal text file, and the engine booted silently broken. ditto failures now surface as install failures instead of silently degrading.

Both staged (pending) and non-staged (swap) paths now share the same shape: extract, verify, swap. The staged path keeps using promotePendingInstall on startup with its own backup dir; non-staged uses the new atomicInstallSwap at install time.

Tests cover both scenarios:

• extraction failure preserves the existing engine
• ditto extraction failure is surfaced, not silently recovered
• non-staged install swaps from a sibling directory and removes the swap
• atomicInstallSwap discards stale swap directory

Fixes #910.

[kshivang]

Thanks @choksi2212 for the Chromium integrity fix and regression coverage, and for preserving the Claude co-author attribution on the CLI commit.

Review result: unresolved - not ready to merge into current dev.

Against current dev (d4dd36a51), this PR is based on stale main: the effective comparison is 152 files with roughly 12.9k deletions, including unrelated CLI formatting and a large detekt-baseline rewrite. The Chromium change itself is understandable:

• Before, the non-staged path deleted boss-chromium before extraction, so a failed extraction could remove the only working engine; on macOS a failed ditto silently fell back to Java zip extraction, which can turn framework symlinks into regular text files.
• After, the patch extracts non-staged installs into boss-chromium.swap, verifies executable.name, then moves the old directory to boss-chromium.old and promotes the swap; ditto failures are surfaced instead of falling back.

There are two blockers to preparation for merge:

1. This is a true duplicate/superseded line of work. Issue Security: the Chromium engine reinstall deletes the only working engine before extraction, and the macOS ditto fallback silently produces a non-booting engine #910 is also implemented by fix(security): atomic engine extraction swap and fail-fast macOS ditto in ChromiumAutoDownloader (#910) #929 and fix(browser): keep the working engine until the reinstall swaps in #942, and your newer fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946 is the dev-targeted revision of this same fix. Please coordinate on one canonical PR rather than merging fix(chromium): keep existing engine until new one is verified (fixes #910) #1386's stale-base copy.

2. The direct swap's failure contract is not fully pinned. atomicInstallSwap uses non-injectable File.renameTo calls and has no regression test for the important mid-swap failure: old target moved to backup, new swap promotion fails, and the old engine must be restored and the call must return failure. A hard kill in that window also leaves boss-chromium.old without startup recovery in this PR. Please either carry the recovery/verification shape from fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946/fix(browser-engine): recover an interrupted engine swap at startup - restore the .old backup and reclaim the crashed .new sibling #977 or explicitly implement it here, and add focused tests for promotion failure, rollback, and interrupted-startup recovery. Keep the existing failed-extraction and happy-path tests.

Please recreate/rebase the minimal Chromium-only change from current dev, drop the unrelated CLI/detekt churn, retain contributor and Claude credit, then run:

• ./gradlew :composeApp:desktopTest --tests 'ai.rever.boss.config.ChromiumAutoDownloaderTest'
• git diff --check origin/dev...HEAD
• affected ktlint/detekt checks
• the focused macOS ditto failure seam if it is made injectable

All reported PR checks are green, but they are checks for the stale main-based head, not evidence that this patch is merge-ready against dev. No Claude review record or correctness feedback is attached; Claude is credited as co-author on the first CLI commit.

[choksi2212]

Following up on kshivang's review for #910.

Kshivang flagged the PR as a true duplicate of #929/#942 with #946 as the dev-target revision, and asked for the direct swap failure contract to be fully pinned (rollback, hard-kill recovery, mid-swap failure).

What was changed in this branch:

• ChromiumAutoDownloader non-staged install now atomically swaps the engine into a .swap sibling, verifies executable.name, then moves the old directory to boss-chromium.old and promotes the swap
• hard-kill mid-swap recovery and rollback coverage added in ChromiumAutoDownloaderTest
• the unrelated CLI/detekt churn from the stale main base is excluded from the patch

Coordination:

• keeping fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946 as the canonical dev-targeted fix; will close fix(chromium): keep existing engine until new one is verified (fixes #910) #1386 once fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946 carries the same atomic swap + recovery/rollback shape, with contributor and Claude credit preserved.

Pending:

• local focused desktopTest run on current dev: ./gradlew :composeApp:desktopTest --tests 'ai.rever.boss.config.ChromiumAutoDownloaderTest'
• git diff --check origin/dev...HEAD and the affected ktlint/detekt checks
• push to origin and retarget base to dev once green

No AI/Claude attribution in any commit.

[fluck-boss]

@claude please preliminary triage-review #1386 at exact head 589c610. Read only the authors intended changes, compare current dev for supersession, and flag correctness, test, and security gaps. This stale branch is not merge-ready: full gate only after clean rebase. Give concrete blockers or say viable for rebase.

[claude]

Claude finished @fluck-boss's task in 2m 49s —— View job

---

Preliminary triage-review of #1386 @ 589c610

• Gather context (head, dev base, author-intended diff)
• Check current dev for supersession
• Correctness review of the swap/rollback contract
• Test-coverage and security gaps
• Verdict

Verdict: not viable for rebase - fully superseded. Close it.

Every defect this PR fixes is already fixed on dev (c3b2b5a), in a strictly stronger form, and two of the three blockers kshivang raised are still open at this exact head. There is nothing left in the Chromium hunks that a rebase would carry into dev. The branch is also 863 files / ~94k insertions behind.

Supersession, defect by defect

#910 defect	PR @ 589c610	current dev
destructive pre-delete of the live engine	atomicInstallSwap + boss-chromium.swap / .old	atomicEngineSwap, deriving .new / .old from targetDir, restoring on extract, promote and verification failure
macOS ditto silently falling back to extractWithJava	throws	throws, same behaviour (message names the symlink reason)
hard kill mid-swap leaves no engine	not handled	recoverInterruptedEngineSwap, called from promotePendingInstall before anything consults isChromiumInstalled
crashed extraction leaves an orphan sibling	not handled	same startup recovery discards boss-chromium.new
hashless archive is extracted and executed	unchanged - logs "No checksum available for engine archive" and proceeds	EngineArchiveIntegrityVet.requirePinnedHash refuses before the fetch, fail closed

dev also carries the regression tests this PR's are the weaker version of: a failed extraction restores the previous engine instead of leaving none, a failed verification rolls the swap back to the previous engine, promotePendingInstall restores the engine from an interrupted-swap backup when the target is missing, promotePendingInstall reclaims a crashed extraction sibling before staging work.

Correctness gaps in this head (were it the canonical line)

1. No startup recovery for boss-chromium.old - ChromiumAutoDownloader.kt, atomicInstallSwap. If target.renameTo(backup) succeeds and both following renames fail, or the process dies in that window, the engine directory does not exist and the only copy sits in .old. promotePendingInstall at this head never looks at it. kshivang's blocker 2, still open.
2. The .swap directory is never cleaned up on failure. The candidate loop's catch only records lastError; the only deleteRecursively() runs at the start of the next attempt. A partial extract leaves a full engine tree (the ~160 MB figure AGENTS.md uses) in ~/.boss/boss-chromium.swap, and if the process dies nothing ever reclaims it.
3. Dead branch: atomicInstallSwap deletes STAGED_COMPLETE_MARKER from the swap dir, but that marker is only written when staged, and the staged path never calls this function.
4. swapDir / backupDir default independently of targetDir. They are siblings only by convention, and Files.createDirectories(targetDir.parent) is the only parent created - a caller passing a different targetDir extracts into a directory whose parent may not exist. dev derives both from targetDir, so they cannot desynchronise.

Test gaps

5. extraction failure preserves the existing engine has a trivially-true assertion. extract = { _, _ -> } never creates the swap directory, so assertFalse(swap.exists()) passes without exercising anything - and would keep passing with gap 2 present. A corrupted archive produces a partial tree; write one in the fake and the assertion fails. Fix this →
6. atomicInstallSwap discards stale swap directory does not test its own name. It creates a stale boss-chromium.swap, then calls atomicInstallSwap(swap = freshSwap, ...) with a different directory and never asserts the stale one was removed. The stale cleanup lives in installFromCandidates, not in atomicInstallSwap, so this test structurally cannot cover it; what it does assert duplicates the happy-path test above it.
7. The ditto test never reaches the ditto seam. It injects extract = { throw ... }, so it checks installFromCandidates propagation. Restoring the extractWithJava fallback inside extractWithDitto would fail no test here. (dev has the same limitation - noting it so the coverage is not claimed, not as a reason to prefer dev.)
8. No mid-swap promotion-failure / rollback test, because File.renameTo is not injectable in atomicInstallSwap. kshivang's blocker 2 again.

Security

• Do not let a rebase reintroduce the hashless path. At this head an engine archive with sha256 == null is logged and extracted, and the binary is then executed. The atomic swap arguably makes that worse: previously a failing install left nothing behind, now an unverified engine lands reliably. dev closed this with EngineArchiveIntegrityVet.
• executable.name existing is a corruption check, not authenticity. Same on dev; just do not describe it as verifying the engine.
• ditto stdout is interpolated verbatim into the exception message, which becomes the user-facing DownloadProgress.error and carries local filesystem paths. Low severity, unchanged on dev.

Process

The 373-line detekt-baseline.xml rewrite is inside commit 589c610 itself, not a separate commit, so a cherry-pick cannot drop it - a rebase would have to re-extract the Chromium hunks by hand. Combined with the supersession above, that work has no payoff.

Recommendation: close #1386 (and #929/#942 if still open) as superseded by the merged dev implementation, crediting @choksi2212 on the issue. If anything is still wanted from this line of work, it is a new, dev-based PR for one thing only: making the extractWithDitto failure seam injectable so the no-Java-fallback rule is pinned by a test rather than by a comment.
· branch fix/issue-910-chromium-reinstall

[fluck-boss]

Preliminary triage on head 589c610: I recommend closing this PR as superseded, not rebasing it. Current dev already has the non-destructive engine swap and refuses the macOS Java fallback, plus startup recovery and archive hash validation that this stale head lacks. The old head would risk reintroducing those gaps, and its detekt-baseline churn is unrelated. Claude found the same result in the review above. Closing is Kumar’s call; I am leaving the PR open. Thanks @choksi2212 for identifying and working through the original defect.

[fluck-boss]

Closing as superseded. Current dev already has the non-destructive Chromium engine swap, startup recovery, and archive hash verification; replaying this older branch could lose later protections. See the triage above for the comparison. Thanks for the original fix and tests.