Compare commits

...
2 Commits
Author SHA1 Message Date
Claude Fable 5 706726a3d4 fix(test): LaunchWiringTests was empirically vacuous for the makeLaunchModel path
The reviewer patched main.swift back to FolderSettings.resolvedAppSupportPaths()
(the AirDrop-only resolver) and `swift test --filter LaunchWiringTests` STILL
PASSED, because the only thing the test asserted — model.watchFolderURL — is
computed independently by AppModel.init() via TransportSettings.effectiveFolders(),
not from the `paths` makeLaunchModel() built. bootstrap()'s own unconditional
updateWatchFolder reconcile then papered over the reverted resolver, so the
test only ever proved the bootstrap reconcile, never the launch resolver
itself. The "verified this catches the blocker" claim in the previous
commit was empirically false.

Fix: added ReturnWatcher.currentWatchFolder (public var, actor-isolated —
the folder a watcher is CURRENTLY seeded to scan, readable without calling
scanNow()/updateWatchFolder first). The test now asserts, BEFORE
bootstrap() runs: model.paths.watchFolder/outbox (already internal-visible
via @testable import, no production API change needed there) equal the
OneDrive folder, AND the watcher's currentWatchFolder equals it too — both
of which genuinely depend on what makeLaunchModel() built.

Verified properly this time (both outputs below are verbatim from
`swift test --filter LaunchWiringTests`, main.swift's makeLaunchModel()
temporarily reverted to FolderSettings.resolvedAppSupportPaths() then
restored — the revert itself is not part of this commit):

FAILURE (reverted resolver):
    Expectation failed: (model.paths.watchFolder.path -> "/Users/benjaminhippler/Downloads")
      == (oneDriveFolder.path -> ".../shotdeck-real-wiring-onedrive-<uuid>")
    Expectation failed: (model.paths.outbox.path -> "/Users/benjaminhippler/Desktop")
      == (oneDriveFolder.path -> ".../shotdeck-real-wiring-onedrive-<uuid>")
    Expectation failed: (seededWatchFolder.path -> "/Users/benjaminhippler/Downloads")
      == (oneDriveFolder.path -> ".../shotdeck-real-wiring-onedrive-<uuid>")
    Test ... failed after 0.324 seconds with 3 issues.

PASS (resolver restored):
    Test "Real wiring: AppDelegate.makeLaunchModel() + AppModel.bootstrap()
      detect a marked OneDrive return" passed after 0.295 seconds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 10:10:14 +04:00
Claude Fable 5 e1f569cc3e fix(core): probeWritable(at:) always cleans up the probe file, even on partial failure
Two leak paths: (a) AtomicFile.write renames the temp file onto the probe
path and THEN fsyncs the containing directory — if that last fsync throws,
the probe file already exists on disk but the old code returned false
without ever attempting removal; (b) if the explicit removeItem call itself
threw, there was no retry, so a transient File Provider removal failure left
the file behind permanently.

Fix: an unconditional `defer` now checks fileExists and retries removeItem
regardless of which branch returned early. The function only reports true
when the explicit write, fsync (inside AtomicFile.write), AND removal all
succeeded AND the file is confirmed gone afterward.

New test: a FileManager subclass whose removeItem(at:) throws on its first
call (AtomicFile.write itself never touches this injected FileManager — it
uses raw Darwin/POSIX calls, not FileManager, so this only intercepts the
explicit removal + the defer's retry) asserts the function returns false AND
no probe file remains — verified this actually needs the defer by
temporarily removing it and confirming the same test then fails with a
leftover ".redline-probe-<uuid>" file (see this branch's history for the
discarded revert). The directory-fsync failure path has no injectable seam
(raw fsync(2) on an already-open fd, not parameterized by any FileManager or
other substitutable dependency, and not reproducible via chmod or other
standard test techniques) — documented in the test rather than simulated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 10:10:00 +04:00
4 changed files with 100 additions and 3 deletions
@@ -17,6 +17,15 @@ public actor ReturnWatcher {
/// A document that IS commented is always recorded, regardless of this flag.
public var recordUncommented: Bool = true
/// The folder this watcher is CURRENTLY seeded to scan/watch whatever `init`
/// last set it to, or `updateWatchFolder` since. Exposed so tests can observe the
/// watcher's seeded folder directly (e.g. right after construction, before
/// `start()`/`updateWatchFolder()` ever run) rather than only inferring it
/// indirectly through `scanNow()`'s behavior.
public var currentWatchFolder: URL {
watchFolder
}
/// Watch folder is `paths.watchFolder`, which production constructs from
/// `FolderSettings.resolve().watch`. This type never calls FolderSettings;
/// `updateWatchFolder` is invoked by the UI layer only.
@@ -189,16 +189,33 @@ public enum OneDriveLocator {
fileManager: FileManager = .default
) -> Bool {
let probeURL = folder.appendingPathComponent(".redline-probe-\(UUID().uuidString)")
// Belt-and-suspenders cleanup, unconditional: AtomicFile.write renames the temp
// file onto probeURL and THEN fsyncs the containing directory if that last
// fsync throws, the probe file already exists on disk but the catch below
// returns false before ever reaching the explicit removeItem call. And if the
// explicit removeItem itself throws, this is the only retry it gets. Either
// way, never leave the probe file behind just because we're about to return.
defer {
if fileManager.fileExists(atPath: probeURL.path) {
try? fileManager.removeItem(at: probeURL)
}
}
do {
try AtomicFile.write(Data(), to: probeURL)
} catch {
return false
}
do {
try fileManager.removeItem(at: probeURL)
} catch {
return false
}
return true
// Only true when the explicit removal above actually succeeded AND the file is
// confirmed gone never trust a removeItem call that returned without throwing
// as proof of anything on a File Provider domain.
return !fileManager.fileExists(atPath: probeURL.path)
}
}
@@ -160,6 +160,49 @@ func probeWritableFalseForAPlainFilePath() throws {
#expect(!OneDriveLocator.probeWritable(at: filePath))
}
/// The write itself succeeds (a real probe file lands on disk via AtomicFile.write,
/// which never touches this injected FileManager it uses raw POSIX calls), but the
/// FIRST call to `removeItem(at:)` throws, simulating a transient File Provider
/// removal failure. probeWritable's own defer-based cleanup must retry and succeed
/// (the second call through this same override falls through to `super`), so no
/// probe file is left behind even though the function correctly still reports false
/// (the removal it explicitly attempted did fail).
private final class ThrowOnceOnRemoveFileManager: FileManager, @unchecked Sendable {
private let lock = NSLock()
private var hasThrown = false
override func removeItem(at URL: URL) throws {
lock.lock()
let shouldThrow = !hasThrown
hasThrown = true
lock.unlock()
if shouldThrow {
throw NSError(domain: "ShotdeckCoreTests.ThrowOnceOnRemove", code: 1)
}
try super.removeItem(at: URL)
}
}
@Test
func probeWritableFalseAndLeavesNoProbeFileWhenRemoveItemThrowsOnce() throws {
// Directory fsync failure (the OTHER way probeWritable's cleanup can be needed) has
// no injectable seam: AtomicFile.write's directory fsync is a raw Darwin fsync(2)
// call on an already-open file descriptor, not parameterized by any FileManager or
// other dependency this test can substitute, and there is no portable way to make
// fsync(2) itself fail via chmod or other standard test techniques (fsync failures
// are OS/filesystem/hardware-level events). Covering the removeItem-throws path
// (below) is what this test does; the fsync-throws path is covered by code
// inspection only the same `defer` block guards both.
let dir = try makeTransportTemporaryDirectory(prefix: "shotdeck-probe-remove-throws")
defer { try? FileManager.default.removeItem(at: dir) }
let injectedFileManager = ThrowOnceOnRemoveFileManager()
#expect(!OneDriveLocator.probeWritable(at: dir, fileManager: injectedFileManager))
let leftovers = try FileManager.default.contentsOfDirectory(atPath: dir.path)
#expect(leftovers.isEmpty)
}
struct TransportDefaultsSuite {
let name: String
let defaults: UserDefaults
+30 -2
View File
@@ -5,14 +5,26 @@ import Testing
import ShotdeckCore
@testable import Shotdeck
/// Coverage gap closed (adversarial review, round 3): the Core-level regression tests
/// in ShotdeckCoreTests hand-replicate what `AppDelegate.makeLaunchModel()` and
/// Coverage gap closed (adversarial review, rounds 3 and 4): the Core-level regression
/// tests in ShotdeckCoreTests hand-replicate what `AppDelegate.makeLaunchModel()` and
/// `AppModel.bootstrap()` do, rather than calling them so a future revert of
/// `makeLaunchModel()` back to the AirDrop-only resolver, or a dropped
/// `updateWatchFolder` call inside `bootstrap()`, would NOT fail `swift test`. This
/// test goes through the real, unmodified call sites in the `Shotdeck` executable
/// target via `@testable import`, which `ShotdeckCoreTests` cannot reach (it only
/// depends on `ShotdeckCore`) hence this separate `ShotdeckTests` target.
///
/// Round 4 correction: the first version of this test asserted only
/// `model.watchFolderURL`, which `AppModel.init` computes independently via
/// `TransportSettings.effectiveFolders()` so it stayed correct (and the test kept
/// passing) even when `makeLaunchModel()` was reverted to the AirDrop-only resolver,
/// because `bootstrap()`'s own unconditional `updateWatchFolder` reconcile papered
/// over the reverted resolver. That made the "verified this catches the blocker"
/// claim in the previous round's commit message empirically false. This version
/// asserts `model.paths`/the watcher's `currentWatchFolder` BEFORE `bootstrap()` runs,
/// which actually depends on what `makeLaunchModel()` built see this file's git
/// history (or the round-4 commit message) for the verbatim before/after
/// `swift test --filter` output proving it now discriminates correctly.
@MainActor
@Test("Real wiring: AppDelegate.makeLaunchModel() + AppModel.bootstrap() detect a marked OneDrive return")
func realLaunchModelAndBootstrapDetectAMarkedOneDriveReturn() async throws {
@@ -73,6 +85,22 @@ func realLaunchModelAndBootstrapDetectAMarkedOneDriveReturn() async throws {
// combo already taken / the global hotkey table in an unexpected state.
defer { model.hotkeys.unregisterAll() }
// PRE-bootstrap assertions this is the actual proof of the launch RESOLVER
// (AppDelegate.makeLaunchModel() -> TransportSettings.resolvedAppSupportPaths()),
// independent of bootstrap()'s own reconcile. `model.watchFolderURL` alone does
// NOT prove this: AppModel.init computes it separately via
// TransportSettings.effectiveFolders(), so it would read as correct even if
// makeLaunchModel's `paths` were built by the AirDrop-only resolver which is
// exactly how the first version of this test was empirically shown to be vacuous
// for the launch-resolver path (see this commit's message). `model.paths` is
// `internal` on AppModel, so @testable import already exposes it without any
// production API change; `currentWatchFolder` is the one new (internal-facing,
// `public` on the actor) seam added to ReturnWatcher for this purpose.
#expect(model.paths.watchFolder.path == oneDriveFolder.path)
#expect(model.paths.outbox.path == oneDriveFolder.path)
let seededWatchFolder = await model.watcher.currentWatchFolder
#expect(seededWatchFolder.path == oneDriveFolder.path)
#expect(model.transport == .oneDrive)
#expect(model.watchFolderURL.path == oneDriveFolder.path)