Compare commits

...
6 Commits
Author SHA1 Message Date
kua-agentandClaude Fable 5.1 70231574c9 test: launch-paths BLOCKER regression + isWritableDirectory unit tests
ReturnWatcherTests.swift: two new tests bracket the BLOCKER fix — one
characterizes the old bug (FolderSettings.resolvedAppSupportPaths(), AirDrop-
only, ignores the persisted OneDrive transport; the watcher ends up watching
a stale isolated folder and misses a marked PDF dropped into the real
OneDrive-mode folder), the other proves the fix (the exact launch/bootstrap
construction — TransportSettings.resolvedAppSupportPaths() +
watcher.updateWatchFolder() before start — detects it). Both isolated to
temp dirs, including an explicit FolderSettings watch-folder override so the
"bug" test's found.isEmpty assertion never depends on what's actually in
Ben's real ~/Downloads (it does, in fact, already contain real marked-up
Redline PDFs from prior testing — an earlier version of this test read the
REAL Downloads folder and failed for exactly that reason).

TransportSettingsTests.swift: three tests for
OneDriveLocator.isWritableDirectory — true for an ordinary directory, false
for one chmod'd 500 (permissions restored in teardown before removal), false
for a plain file and for a nonexistent path.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:28:17 +04:00
kua-agentandClaude Fable 5.1 8121738188 fix: MINOR — ONEDRIVE-SELFTEST resolves the real sync root at runtime; adds relaunch + rapid-toggle sub-steps
realOneDriveSyncRoot was a hardcoded /Users/benjaminhippler/... literal.
Now resolved via OneDriveLocator.syncRoots().first at runtime (MMD-named
root still preferred, matching production); when no OneDrive sync root
exists at all, prints "ONEDRIVE-SELFTEST SKIP no OneDrive sync root" and
exits non-zero — never a false PASS.

Adds two sub-steps to the same phase, both required by the BLOCKER fix's
review: (1) rapid transport toggling (chooseTransport(.airDrop) immediately
followed by chooseTransport(.oneDrive)) must still end with the watcher
watching the OneDrive folder — proves the generation-guarded reconcile in
SettingsView.swift really lets the last choice win. (2) relaunch simulation
— OneDrive still persisted from sub-step 1, a FRESH model built via the
exact same AppDelegate.makeLaunchModel() function real launch uses (now
internal + an appSupportRoot override for this purpose, temp-rooted so this
never touches the real ~/Library/Application Support/Shotdeck), bootstrapped,
then a PDF marked up in place — the relaunched watcher must report it. This
is the exact BLOCKER scenario the review flagged, proven end to end.

Factored the ink-annotation and unmarked-PDF-writing code into
addInkMark(to:)/writeUnmarkedRedlinePDF(to:) so both new sub-steps and the
original markup check share it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:28:08 +04:00
kua-agentandClaude Fable 5.1 6484fde530 fix: refuse transport changes mid-send; generation-guard rapid toggling; one-line "not found" row
MAJOR: chooseTransport/chooseOneDriveFolder now refuse (status "Finish the
current send first.") while isSending is true, closing off the send-vs-
transport-switch race at the UI entry point (SendController.swift's commit
in this same series is the structural fix underneath).

MINOR: both functions spawned unstructured `Task { }` calls to
watcher.setRecordUncommented/updateWatchFolder; rapid toggling could apply
an earlier, superseded call's folder/flag after a later one had already won.
Fixed with a monotonically increasing `reconcileGeneration` counter
(AppModel.swift) bumped synchronously before each Task starts; the Task
checks its own snapshot against the live value before every mutating step
(not just once via Task.isCancelled), so the LAST choice always wins.
Verified via ONEDRIVE-SELFTEST's new rapid-toggle sub-step (this branch's
PickerSelfTest.swift commit), which proves the watcher ends up watching the
folder from the last chooseTransport call.

Design fix (Ben, panel-08 review): the "No OneDrive folder found — sign in to
OneDrive or choose a folder." value text wrapped over four lines, making that
row tall and ragged next to Choose…. The value column now reads exactly "Not
found" (secondary colour, one line, same as the truncated-path style), and
the explanation moves to the caption below: "No OneDrive folder found. Sign
in to OneDrive, or choose a folder." When a folder IS resolved the caption is
unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:27:58 +04:00
kua-agentandClaude Fable 5.1 bd11ba96c5 fix: MAJOR — send() race between concurrent transport switch and an in-flight send
Two issues: (1) directoryExists(at:) only checked existence + isDirectory, so
an existing-but-unwritable OneDrive folder skipped the
oneDriveFolderUnavailable branch and surfaced as a generic pdfCompositionFailed
message instead — now uses OneDriveLocator.isWritableDirectory(at:). (2)
send() read `self.outboxURL` again inside composePDFForSend() after at least
one await had already run, so a concurrent chooseTransport() call
(SettingsView.swift) could flip transport/outboxURL/recordUncommented
mid-send, landing the PDF under one transport's folder while the
archive/status branch ran the other's.

Fix: send() now snapshots BOTH transport and the destination folder into
local `let`s once, before any await, and passes the folder explicitly into
the renamed composePDFForSend(outbox:) — which no longer reads
self.outboxURL at all. The archive/status switch already used the frozen
`transport` local. (chooseTransport/chooseOneDriveFolder additionally refuse
outright while isSending is true — see the SettingsView.swift commit — so in
practice this race can no longer even be triggered, but the snapshot is the
actual structural fix regardless.)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:27:49 +04:00
kua-agentandClaude Fable 5.1 ef0d8712f7 fix: BLOCKER — launch model uses transport-aware paths; bootstrap reconciles watcher folder unconditionally
AppDelegate.makeLaunchModel() built `paths` via the AirDrop-only
FolderSettings.resolvedAppSupportPaths(), so ReturnWatcher's internal
watchFolder (seeded from paths.watchFolder in its own init) was the AirDrop
folder even when OneDrive was the persisted transport, and bootstrap() never
reconciled it before starting. Net effect: on every relaunch with OneDrive
selected, PDFs went to OneDrive but FSEvents kept watching the stale AirDrop
folder for the whole session — marked-up returns were never detected.

Fix: makeLaunchModel() now calls TransportSettings.resolvedAppSupportPaths()
(single source of truth for the transport-folder mapping); made internal
(not private) with an optional appSupportRoot override so
PickerSelfTest's relaunch-simulation sub-step can call the exact same
function against a temp root instead of the real Application Support folder.
bootstrap() now unconditionally calls watcher.updateWatchFolder(watchFolderURL)
before watcher.start() (belt-and-suspenders reconciliation, even though the
paths fix alone already makes this a no-op in the normal case), and sets
recordUncommented before start as it already did.

Regression tests proving this land in the same PR (ReturnWatcherTests.swift):
one characterizing the old bug's exact construction still missing a marked
OneDrive-mode return, one proving the fixed launch-construction path detects
it end to end. The ONEDRIVE-SELFTEST phase also gains a relaunch sub-step
using this same makeLaunchModel() function.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:27:42 +04:00
kua-agentandClaude Fable 5.1 20de467e87 fix(core): transport-aware launch paths + writable-folder check (adversarial review)
BLOCKER fix, Core half: adds TransportSettings.resolvedAppSupportPaths(),
the transport-aware equivalent of the AirDrop-only
FolderSettings.resolvedAppSupportPaths() — launch code must use this one so
the ReturnWatcher it feeds is never seeded with a stale AirDrop folder while
OneDrive is the persisted transport. Both now share a single
AppSupportPaths.standardRoot() helper for the ~/Library/Application
Support/Shotdeck root, instead of computing it three separate times.

MAJOR fix, Core half: adds OneDriveLocator.isWritableDirectory(at:) — exists
+ isDirectory is not enough; an existing-but-unwritable folder (permissions
revoked) must be treated as unavailable, not silently attempted and surfaced
as a generic PDF-composition failure.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZiTXPbPCSjzPVsfoweAbp
2026-09-05 09:27:32 +04:00
10 changed files with 412 additions and 86 deletions
+18
View File
@@ -132,6 +132,13 @@ public final class AppModel {
}
func setTransport(_ value: SendTransport) { transport = value }
func setResolvedOneDriveFolder(_ value: URL?) { resolvedOneDriveFolder = value }
/// Bumped by chooseTransport/chooseOneDriveFolder (SettingsView.swift) before each
/// spawns its async watcher-reconcile Task; that Task checks its own snapshot
/// against the live value before every mutating step, so rapid toggling always
/// lets the LAST choice win instead of applying stale, superseded work. Not
/// `@Observable`-relevant state pure internal bookkeeping, never read by a View.
var reconcileGeneration = 0
func rememberLastComposedPDF(_ url: URL) { lastComposedPDFURL = url }
/// True when a last-composed PDF path is known this run, or the newest
@@ -211,6 +218,17 @@ public final class AppModel {
await watcher.setRecordUncommented(transport == .airDrop)
do {
// BLOCKER fix: reconcile the watcher's internal watchFolder with the live
// watchFolderURL UNCONDITIONALLY, before it ever starts. `paths` (and so the
// watcher's initial folder, set in its own init) now comes from the same
// transport-aware TransportSettings.effectiveFolders() as watchFolderURL, so
// in the normal case this is a no-op but it is the only thing that would
// have caught the old bug (launch paths built AirDrop-only while OneDrive was
// the persisted transport, leaving the watcher's FSEvents stream pointed at a
// stale folder for the whole session) and it stays cheap insurance against
// that class of drift ever recurring. Calling it before start() only updates
// the stored folder no FSEvents stream exists yet to restart.
try await watcher.updateWatchFolder(watchFolderURL)
try await watcher.start { [weak self] _ in
Task { @MainActor in
guard let self else { return }
+129 -41
View File
@@ -223,7 +223,7 @@ enum PickerSelfTest {
sendTruthFail("seeded session was empty")
}
let pending = try await model.composePDFForSend()
let pending = try await model.composePDFForSend(outbox: model.outboxURL)
guard fm.fileExists(atPath: pending.fileURL.path) else {
sendTruthFail("PDF was not written")
}
@@ -476,18 +476,13 @@ enum PickerSelfTest {
exit(1)
}
/// Real sync root this Mac has; the phase proves the transport against the actual
/// OneDrive file provider, never a fake home tree (that is what
/// OneDriveLocatorTests in ShotdeckCoreTests are for).
private static let realOneDriveSyncRoot = URL(
fileURLWithPath: "/Users/benjaminhippler/Library/CloudStorage/OneDrive-MMDGROUP",
isDirectory: true
)
/// Phase 5: proves the OneDrive transport end to end against the real sync root.
/// Triggered by `SHOTDECK_ONEDRIVE_SELFTEST` when chained after PICKER/SEND-TRUTH/
/// UPDATE-SELFTEST the exact pattern `startUpdateSelfTestIfRequested` uses for its
/// own env var. Returns true when the async phase was scheduled (it calls `exit` itself).
/// Phase 5: proves the OneDrive transport end to end against a REAL sync root
/// resolved at runtime via `OneDriveLocator.syncRoots()`, never a hardcoded path, so
/// this runs correctly on any Mac/account that has OneDrive signed in (MMD-named
/// root preferred, same as production). Triggered by `SHOTDECK_ONEDRIVE_SELFTEST`
/// when chained after PICKER/SEND-TRUTH/UPDATE-SELFTEST the exact pattern
/// `startUpdateSelfTestIfRequested` uses for its own env var. Returns true when the
/// async phase was scheduled (it calls `exit` itself).
@discardableResult
private static func startOneDriveSelfTestIfRequested() -> Bool {
guard ProcessInfo.processInfo.environment["SHOTDECK_ONEDRIVE_SELFTEST"] != nil else {
@@ -514,9 +509,16 @@ enum PickerSelfTest {
}
private static func runOneDriveSelfTestAndExit() {
// Never a false PASS: no real OneDrive sync root on this machine/account is a
// SKIP (still non-zero exit), not silently treated as passing.
guard let syncRoot = OneDriveLocator.syncRoots().first else {
print("ONEDRIVE-SELFTEST SKIP no OneDrive sync root")
fflush(stdout)
exit(1)
}
Task { @MainActor in
do {
let folder = try await executeOneDriveSelfTest()
let folder = try await executeOneDriveSelfTest(syncRoot: syncRoot)
print("ONEDRIVE-SELFTEST PASS path=\(folder.path)")
fflush(stdout)
exit(0)
@@ -528,19 +530,30 @@ enum PickerSelfTest {
}
}
/// Builds a session, sends it through the OneDrive branch of `send(anchor: nil)`
/// against a NEW folder under the real OneDrive sync root, confirms the watcher does
/// NOT report the freshly-written unmarked PDF as a return, then adds a real PDFKit
/// ink annotation in place (what the iPad does) and confirms the watcher now reports
/// it as commented. Never deletes anything under OneDrive the created folder and
/// PDF are left in place for Ben to inspect / for the real iPad round trip.
private static func executeOneDriveSelfTest() async throws -> URL {
/// Sub-step 1: builds a session, sends it through the OneDrive branch of
/// `send(anchor: nil)` against a NEW folder under `syncRoot`, confirms the watcher
/// does NOT report the freshly-written unmarked PDF as a return, then adds a real
/// PDFKit ink annotation in place (what the iPad does) and confirms the watcher now
/// reports it as commented.
///
/// Sub-step 1b: rapid transport toggling (chooseTransport(.airDrop) immediately
/// followed by chooseTransport(.oneDrive), no await between them) must still end
/// with the watcher pointed at the OneDrive folder proves the generation-guarded
/// reconcile in chooseTransport/chooseOneDriveFolder (SettingsView.swift) really
/// does let the last choice win instead of an earlier, superseded call applying its
/// stale folder after a later one already won.
///
/// Sub-step 2: relaunch simulation the exact BLOCKER scenario this phase exists to
/// catch. OneDrive is still persisted in defaults from sub-step 1; builds a FRESH
/// model the same way the real app launches (`AppDelegate.makeLaunchModel()` itself,
/// not a reimplementation), bootstraps it, then marks a PDF in the folder in place
/// the relaunched watcher must report it. A temp app-support root keeps this off the
/// real ~/Library/Application Support/Shotdeck.
///
/// Never deletes anything under OneDrive the created folder and PDFs are left in
/// place for Ben to inspect / for the real iPad round trip.
private static func executeOneDriveSelfTest(syncRoot: URL) async throws -> URL {
let fm = FileManager.default
guard fm.fileExists(atPath: realOneDriveSyncRoot.path) else {
throw OneDriveSelfTestError.detail(
"real OneDrive sync root not found at \(realOneDriveSyncRoot.path)"
)
}
// UserDefaults.standard is the ONLY defaults instance send()/TransportSettings
// actually read at runtime (there is no defaults-threading through AppModel), so
@@ -563,8 +576,7 @@ enum PickerSelfTest {
}
let stamp = DubaiTime.fileStamp(Date())
let selftestFolder = realOneDriveSyncRoot
.appendingPathComponent("Redline-selftest-\(stamp)", isDirectory: true)
let selftestFolder = syncRoot.appendingPathComponent("Redline-selftest-\(stamp)", isDirectory: true)
try fm.createDirectory(at: selftestFolder, withIntermediateDirectories: true)
TransportSettings.setTransport(.oneDrive, defaults: defaults)
@@ -629,8 +641,77 @@ enum PickerSelfTest {
}
// What the iPad does: mark it up in place with a real ink annotation, then save.
guard let document = PDFDocument(url: pdfURL), let page = document.page(at: 0) else {
throw OneDriveSelfTestError.detail("could not reopen \(pdfURL.path) to annotate it")
try addInkMark(to: pdfURL)
let afterMarkup = try await watcher.scanNow()
guard let recorded = afterMarkup.first(where: { $0.fileURL == pdfURL }), recorded.isCommented else {
throw OneDriveSelfTestError.detail("annotated PDF was not reported as commented by scanNow")
}
let commentedAfter = try await ledger.commented()
guard commentedAfter.contains(where: { $0.fileURL == pdfURL }) else {
throw OneDriveSelfTestError.detail("annotated PDF was not recorded in the ledger as commented")
}
// Sub-step 1b: rapid toggle race see the doc comment above this function.
model.chooseTransport(.airDrop)
model.chooseTransport(.oneDrive) // immediately superseding the call above
// The generation guard itself is what's under test, not this wait it just
// gives the (already-guarded) reconcile Task a moment to settle either way.
try await Task.sleep(for: .milliseconds(500))
guard model.transport == .oneDrive else {
throw OneDriveSelfTestError.detail(
"rapid toggle: model.transport ended as \(model.transport), expected .oneDrive"
)
}
let racePDFURL = selftestFolder.appendingPathComponent("Redline-race-\(stamp).pdf")
try writeUnmarkedRedlinePDF(to: racePDFURL)
try addInkMark(to: racePDFURL)
let raceFound = try await model.watcher.scanNow()
guard raceFound.first(where: { $0.fileURL == racePDFURL })?.isCommented == true else {
throw OneDriveSelfTestError.detail(
"rapid toggle: watcher did not end up watching \(selftestFolder.path) — an earlier, superseded chooseTransport call won"
)
}
// Sub-step 2: relaunch simulation see the doc comment above this function.
let relaunchAppSupportRoot = fm.temporaryDirectory
.appendingPathComponent("shotdeck-onedrive-relaunch-\(UUID().uuidString)", isDirectory: true)
defer { try? fm.removeItem(at: relaunchAppSupportRoot) }
let relaunchModel = AppDelegate.makeLaunchModel(appSupportRoot: relaunchAppSupportRoot)
guard relaunchModel.transport == .oneDrive else {
throw OneDriveSelfTestError.detail(
"relaunch: model transport was \(relaunchModel.transport), expected .oneDrive"
)
}
guard relaunchModel.watchFolderURL.path == selftestFolder.path else {
throw OneDriveSelfTestError.detail(
"relaunch: model watchFolderURL was \(relaunchModel.watchFolderURL.path), expected \(selftestFolder.path) — this is the exact BLOCKER this phase guards against"
)
}
await relaunchModel.bootstrap()
let relaunchPDFURL = selftestFolder.appendingPathComponent("Redline-relaunch-\(stamp).pdf")
try writeUnmarkedRedlinePDF(to: relaunchPDFURL)
try addInkMark(to: relaunchPDFURL)
let relaunchFound = try await relaunchModel.watcher.scanNow()
guard relaunchFound.first(where: { $0.fileURL == relaunchPDFURL })?.isCommented == true else {
throw OneDriveSelfTestError.detail(
"relaunch: watcher did not report the marked PDF at \(relaunchPDFURL.path) as returned — it was watching the wrong folder after relaunch"
)
}
await relaunchModel.watcher.stop()
return selftestFolder
}
/// Adds a real PDFKit ink annotation to the PDF at `url` in place and saves it
/// exactly what the iPad does when marking up a page.
private static func addInkMark(to url: URL) throws {
guard let document = PDFDocument(url: url), let page = document.page(at: 0) else {
throw OneDriveSelfTestError.detail("could not reopen \(url.path) to annotate it")
}
let ink = PDFAnnotation(
bounds: CGRect(x: 20, y: 20, width: 60, height: 60),
@@ -642,20 +723,27 @@ enum PickerSelfTest {
stroke.line(to: NSPoint(x: 80, y: 80))
ink.add(stroke)
page.addAnnotation(ink)
guard document.write(to: pdfURL) else {
throw OneDriveSelfTestError.detail("could not save the annotated PDF back to \(pdfURL.path)")
guard document.write(to: url) else {
throw OneDriveSelfTestError.detail("could not save the annotated PDF back to \(url.path)")
}
}
let afterMarkup = try await watcher.scanNow()
guard let recorded = afterMarkup.first(where: { $0.fileURL == pdfURL }), recorded.isCommented else {
throw OneDriveSelfTestError.detail("annotated PDF was not reported as commented by scanNow")
/// Writes a fresh, unmarked, single-page "Redline"-creator PDF straight to `url`
/// standing in for a PDF that has just landed in the watch folder, before any
/// human mark. Used by the rapid-toggle and relaunch sub-steps, which don't need to
/// exercise send()/composePDFForSend() again (sub-step 1 already does).
private static func writeUnmarkedRedlinePDF(to url: URL) throws {
let document = PDFDocument()
let page = PDFPage()
page.setBounds(CGRect(x: 0, y: 0, width: 612, height: 792), for: .mediaBox)
document.insert(page, at: 0)
document.documentAttributes = [
PDFDocumentAttribute.creatorAttribute: "Redline",
PDFDocumentAttribute.subjectAttribute: UUID().uuidString,
]
guard document.write(to: url) else {
throw OneDriveSelfTestError.detail("could not write \(url.path)")
}
let commentedAfter = try await ledger.commented()
guard commentedAfter.contains(where: { $0.fileURL == pdfURL }) else {
throw OneDriveSelfTestError.detail("annotated PDF was not recorded in the ledger as commented")
}
return selftestFolder
}
private static func oneDriveFail(_ detail: String) -> Never {
+27 -18
View File
@@ -16,17 +16,28 @@ extension AppModel: SendCapable {
guard !session.isEmpty, !isSending else { return }
setSending(true)
// Snapshot BOTH the transport AND the destination folder into local `let`s
// ONCE, before any `await` in this function. chooseTransport/chooseOneDriveFolder
// now refuse (status "Finish the current send first.") while isSending is true,
// but this snapshot is the actual fix for the race: even without that guard,
// everything below operates on these frozen values composePDFForSend(outbox:)
// takes the folder as a parameter and never re-reads `self.outboxURL` after a
// suspension point, so a concurrent transport switch mid-send can no longer land
// the PDF under one transport's folder while the archive/status branch (which
// switches on the same frozen `transport` local) runs the other's.
let transport = TransportSettings.transport()
let destinationFolder: URL
// OneDrive mode: verify the real destination exists RIGHT NOW, before composing
// anything. `outboxURL` is kept in sync with the resolved OneDrive folder by
// bootstrap/chooseTransport/chooseOneDriveFolder, but this is re-resolved fresh
// here (never trusted stale) so a folder that vanished since then (OneDrive
// signed out, external volume unmounted, folder deleted) is caught instead of
// silently writing into whatever `outboxURL` happens to hold.
// OneDrive mode: verify the real destination exists AND is writable RIGHT NOW,
// before composing anything. `outboxURL` is kept in sync with the resolved
// OneDrive folder by bootstrap/chooseTransport/chooseOneDriveFolder, but this is
// re-resolved fresh here (never trusted stale) so a folder that vanished or lost
// its permissions since then (OneDrive signed out, external volume unmounted,
// folder deleted, chmod'd unwritable) is caught instead of silently attempted
// and surfacing as a generic PDF-composition failure.
if transport == .oneDrive {
guard let folder = OneDriveLocator.resolveOneDriveFolder(),
Self.directoryExists(at: folder)
OneDriveLocator.isWritableDirectory(at: folder)
else {
let path = OneDriveLocator.resolveOneDriveFolder()?.path
?? TransportSettings.storedOneDriveFolderPath()
@@ -36,16 +47,19 @@ extension AppModel: SendCapable {
setSending(false)
return
}
destinationFolder = folder
setResolvedOneDriveFolder(folder)
if outboxURL != folder || watchFolderURL != folder {
setFolderURLs(outbox: folder, watch: folder)
try? await watcher.updateWatchFolder(folder)
}
} else {
destinationFolder = outboxURL
}
let pending: ComposedSend
do {
pending = try await composePDFForSend()
pending = try await composePDFForSend(outbox: destinationFolder)
} catch {
// Never unlink the published PDF, and never unlink the temp file either:
// a rename failure would leave the complete document at the temp name.
@@ -93,19 +107,14 @@ extension AppModel: SendCapable {
}
}
private static func directoryExists(at url: URL) -> Bool {
var isDirectory: ObjCBool = false
let exists = FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory)
return exists && isDirectory.boolValue
}
/// Writes the PDF to the outbox and records its path. Does not archive the session
/// Writes the PDF to `outboxDir` and records its path. Does not archive the session
/// and does not present AirDrop that happens only after the share completes.
func composePDFForSend() async throws -> ComposedSend {
/// `outboxDir` is passed in (a value `send(anchor:)` snapshotted before any await)
/// rather than read from `self.outboxURL` here, so a concurrent transport switch
/// mid-send can never redirect an in-flight compose to a different folder.
func composePDFForSend(outbox outboxDir: URL) async throws -> ComposedSend {
let workingSession = session
let composer = self.composer
// Live outbox (FolderSettings), not `paths.outbox` Settings changes take effect.
let outboxDir = outboxURL
let sourceDir = paths.sessionDirectory(workingSession.id)
let fileName = PDFComposer.fileName(for: workingSession)
let finalURL = outboxDir.appendingPathComponent(fileName)
+37 -9
View File
@@ -82,21 +82,20 @@ struct SettingsView: View {
model.chooseOneDriveFolder()
}
} else {
HStack(spacing: 8) {
Text("No OneDrive folder found — sign in to OneDrive or choose a folder.")
.font(.caption)
.foregroundStyle(.secondary)
.fixedSize(horizontal: false, vertical: true)
.frame(maxWidth: .infinity, alignment: .leading)
Button("Choose…") { model.chooseOneDriveFolder() }
// One-line row, same shape as the normal path row: "Not found"
// where the path would be, Choose stays live. The explanation
// moves to the caption below instead of wrapping this row.
folderValue(path: "Not found") {
model.chooseOneDriveFolder()
}
.frame(minHeight: 22)
}
}
GridRow {
Text(
"The PDF is saved here and this same folder is watched for the marked-up copy. On the iPad open it from Files > OneDrive."
model.resolvedOneDriveFolder != nil
? "The PDF is saved here and this same folder is watched for the marked-up copy. On the iPad open it from Files > OneDrive."
: "No OneDrive folder found. Sign in to OneDrive, or choose a folder."
)
.font(.caption)
.foregroundStyle(.secondary)
@@ -249,7 +248,18 @@ extension AppModel: SettingsWindowPresenting {
/// doesn't exist yet, and re-points the running watcher (folder + recordUncommented)
/// at the new state. Switching back to AirDrop restores its own stored overrides
/// untouched, since AirDrop and OneDrive folder settings are stored under separate keys.
/// Refuses while a send is in flight (send() snapshots its own folder/transport, but
/// switching mid-send is still confusing UX nothing to gain by allowing it).
/// The async reconcile below is generation-guarded: `reconcileGeneration` is bumped
/// synchronously before the Task starts, and the Task checks its own snapshot against
/// the live value before every mutating step, so rapid toggling (this function or
/// chooseOneDriveFolder, in any order) always lets the LAST choice win instead of an
/// earlier, superseded call applying its stale folder/flag after a later one already won.
func chooseTransport(_ value: SendTransport) {
guard !isSending else {
setStatus("Finish the current send first.")
return
}
guard value != transport else { return }
TransportSettings.setTransport(value)
setTransport(value)
@@ -261,11 +271,17 @@ extension AppModel: SettingsWindowPresenting {
}
setFolderURLs(outbox: folders.outbox, watch: folders.watch)
setResolvedOneDriveFolder(OneDriveLocator.resolveOneDriveFolder())
reconcileGeneration += 1
let generation = reconcileGeneration
Task {
guard generation == self.reconcileGeneration else { return }
await watcher.setRecordUncommented(value == .airDrop)
guard generation == self.reconcileGeneration else { return }
do {
try await watcher.updateWatchFolder(folders.watch)
} catch {
guard generation == self.reconcileGeneration else { return }
setStatus(
(error as? ShotdeckError)?.errorDescription ?? "Could not switch the watch folder."
)
@@ -273,7 +289,13 @@ extension AppModel: SettingsWindowPresenting {
}
}
/// Refuses while a send is in flight, same reasoning as chooseTransport. See
/// chooseTransport's doc comment for the generation-guard mechanism shared here.
func chooseOneDriveFolder() {
guard !isSending else {
setStatus("Finish the current send first.")
return
}
let start = resolvedOneDriveFolder ?? FileManager.default.homeDirectoryForCurrentUser
guard let url = chooseDirectory(startingAt: start) else { return }
TransportSettings.setOneDriveFolder(url)
@@ -281,11 +303,17 @@ extension AppModel: SettingsWindowPresenting {
setResolvedOneDriveFolder(OneDriveLocator.resolveOneDriveFolder())
guard transport == .oneDrive else { return }
setFolderURLs(outbox: url, watch: url)
reconcileGeneration += 1
let generation = reconcileGeneration
Task {
guard generation == self.reconcileGeneration else { return }
do {
try await watcher.updateWatchFolder(url)
guard generation == self.reconcileGeneration else { return }
setStatus("OneDrive folder set to \(url.lastPathComponent).")
} catch {
guard generation == self.reconcileGeneration else { return }
setStatus(
(error as? ShotdeckError)?.errorDescription ?? "Could not switch the watch folder."
)
+11 -2
View File
@@ -50,9 +50,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
Task { await model.bootstrap() }
}
private static func makeLaunchModel() -> AppModel {
/// Builds the model exactly the way the real app launches: paths come from
/// `TransportSettings.resolvedAppSupportPaths()` transport-aware, so the watcher
/// this feeds is never seeded with a stale AirDrop folder while OneDrive is the
/// persisted transport (that was the BLOCKER this function used to have, when it
/// called the AirDrop-only `FolderSettings.resolvedAppSupportPaths()` instead).
/// `appSupportRoot` exists only so PickerSelfTest's relaunch-simulation sub-step can
/// point this at a temp directory instead of the real
/// ~/Library/Application Support/Shotdeck production always calls this with no
/// argument (the real root). Internal, not private, for that same reason.
static func makeLaunchModel(appSupportRoot: URL? = nil) -> AppModel {
do {
let paths = try FolderSettings.resolvedAppSupportPaths()
let paths = try TransportSettings.resolvedAppSupportPaths(root: appSupportRoot)
return try makeModel(paths: paths)
} catch {
Log.ui.critical(
@@ -11,12 +11,6 @@ public struct AppSupportPaths: Sendable {
/// Production paths.
public static func standard() throws -> AppSupportPaths {
let fileManager = FileManager.default
let appSupportParent = try fileManager.url(
for: .applicationSupportDirectory,
in: .userDomainMask,
appropriateFor: nil,
create: true
)
let desktop = try fileManager.url(
for: .desktopDirectory,
in: .userDomainMask,
@@ -29,10 +23,25 @@ public struct AppSupportPaths: Sendable {
appropriateFor: nil,
create: true
)
let root = appSupportParent.appendingPathComponent("Shotdeck", isDirectory: true)
let root = try standardRoot(fileManager: fileManager)
return try AppSupportPaths(root: root, outbox: desktop, watchFolder: downloads)
}
/// The standard `~/Library/Application Support/Shotdeck` root. Shared by
/// `standard()`, `FolderSettings.resolvedAppSupportPaths()`, and
/// `TransportSettings.resolvedAppSupportPaths()` so all three agree on where the
/// root lives the folder-resolution logic (AirDrop-only vs transport-aware)
/// differs between those, the root computation never should.
public static func standardRoot(fileManager: FileManager = .default) throws -> URL {
let appSupportParent = try fileManager.url(
for: .applicationSupportDirectory,
in: .userDomainMask,
appropriateFor: nil,
create: true
)
return appSupportParent.appendingPathComponent("Shotdeck", isDirectory: true)
}
/// Test paths rooted anywhere. Every directory is created if missing.
public init(root: URL, outbox: URL, watchFolder: URL) throws {
self.root = root
@@ -70,15 +70,7 @@ public enum FolderSettings {
defaults: UserDefaults = .standard,
fileManager: FileManager = .default
) throws -> AppSupportPaths {
let resolvedRoot: URL
if let root {
resolvedRoot = root
} else {
let appSupportParent = try fileManager.url(
for: .applicationSupportDirectory, in: .userDomainMask,
appropriateFor: nil, create: true)
resolvedRoot = appSupportParent.appendingPathComponent("Shotdeck", isDirectory: true)
}
let resolvedRoot = try root ?? AppSupportPaths.standardRoot(fileManager: fileManager)
let folders = resolve(defaults: defaults, fileManager: fileManager)
return try AppSupportPaths(root: resolvedRoot, outbox: folders.outbox, watchFolder: folders.watch)
}
@@ -75,6 +75,25 @@ public enum TransportSettings {
return (folders.outbox, folders.watch, transport)
}
}
/// Builds an `AppSupportPaths` using `root` (defaults to the standard
/// `~/Library/Application Support/Shotdeck` when nil) plus whatever
/// `effectiveFolders()` returns for outbox/watch. Unlike
/// `FolderSettings.resolvedAppSupportPaths()` (AirDrop-only), this is
/// transport-aware it is the ONLY function launch code should use to build its
/// paths, so the watcher it feeds is never seeded with a stale AirDrop folder while
/// OneDrive is the persisted transport. `root` is exposed purely so tests (and the
/// ONEDRIVE-SELFTEST relaunch simulation) can point it at a temporary directory
/// instead of the user's real Application Support folder.
public static func resolvedAppSupportPaths(
root: URL? = nil,
defaults: UserDefaults = .standard,
fileManager: FileManager = .default
) throws -> AppSupportPaths {
let resolvedRoot = try root ?? AppSupportPaths.standardRoot(fileManager: fileManager)
let folders = effectiveFolders(defaults: defaults, fileManager: fileManager)
return try AppSupportPaths(root: resolvedRoot, outbox: folders.outbox, watchFolder: folders.watch)
}
}
/// Pure path logic for locating a OneDrive sync root under
@@ -142,4 +161,19 @@ public enum OneDriveLocator {
}
return defaultRedlineFolder(home: home, fileManager: fileManager)
}
/// True when `url` exists as a directory AND is writable by the current process.
/// The live check `send(anchor:)` performs before ever composing into a OneDrive
/// destination a directory that exists but has had its permissions revoked (e.g.
/// `chmod 500`) must be treated as unavailable, not silently attempted and
/// surfaced as a generic PDF-composition failure.
public static func isWritableDirectory(
at url: URL,
fileManager: FileManager = .default
) -> Bool {
var isDirectory: ObjCBool = false
let exists = fileManager.fileExists(atPath: url.path, isDirectory: &isDirectory)
guard exists, isDirectory.boolValue else { return false }
return fileManager.isWritableFile(atPath: url.path)
}
}
@@ -282,3 +282,110 @@ func recordUncommentedFalseSkipsUnmarkedThenRecordsAfterInPlaceMarkup() async th
#expect(commented.count == 1)
#expect(commented.first?.fileURL.resolvingSymlinksInPath().path == pdfURL.resolvingSymlinksInPath().path)
}
// MARK: - Launch-paths BLOCKER regression (adversarial review, 20260905)
//
// The bug: AppDelegate.makeLaunchModel() built `paths` via the AirDrop-only
// FolderSettings.resolvedAppSupportPaths(), so ReturnWatcher's internal watchFolder
// (seeded from paths.watchFolder in its own init) was the AirDrop folder even when
// OneDrive was the persisted transport, and bootstrap() never reconciled it before
// starting. Net effect: PDFs went to OneDrive but FSEvents kept watching the stale
// AirDrop folder for the whole session marked-up returns were never detected.
// The fix: launch paths now come from TransportSettings.resolvedAppSupportPaths()
// (transport-aware), and AppModel.bootstrap() unconditionally reconciles the watcher's
// folder via updateWatchFolder() before it starts. These two tests characterize the
// bug (still reproducible via the old AirDrop-only construction) and prove the fix
// (the real launch-construction path, end to end).
@Test("Launch regression (fix): OneDrive persisted -> transport-aware launch paths -> bootstrap-style reconcile -> a marked PDF is detected")
func launchStyleConstructionWithOneDriveTransportDetectsAMarkedReturn() async throws {
let suite = try makeTransportDefaultsSuite()
defer { tearDownTransportSuite(suite) }
let oneDriveFolder = try makeTransportTemporaryDirectory(prefix: "shotdeck-launch-onedrive")
defer { try? FileManager.default.removeItem(at: oneDriveFolder) }
let appSupportRoot = try makeTransportTemporaryDirectory(prefix: "shotdeck-launch-approot")
defer { try? FileManager.default.removeItem(at: appSupportRoot) }
TransportSettings.setTransport(.oneDrive, defaults: suite.defaults)
TransportSettings.setOneDriveFolder(oneDriveFolder, defaults: suite.defaults)
// Exactly what AppDelegate.makeLaunchModel() now does: build launch paths from the
// transport-aware resolver the fix, NOT FolderSettings.resolvedAppSupportPaths(),
// which is AirDrop-only and is the root cause the next test characterizes.
let paths = try TransportSettings.resolvedAppSupportPaths(
root: appSupportRoot, defaults: suite.defaults, fileManager: .default
)
#expect(paths.outbox.path == oneDriveFolder.path)
#expect(paths.watchFolder.path == oneDriveFolder.path)
let ledger = try ReturnLedger(paths: paths)
let watcher = ReturnWatcher(paths: paths, ledger: ledger)
// What AppModel.bootstrap() now does, unconditionally, before watcher.start():
await watcher.setRecordUncommented(false) // transport == .oneDrive
try await watcher.updateWatchFolder(paths.watchFolder)
let pdfURL = oneDriveFolder.appendingPathComponent("Redline-20260905-100000.pdf")
try makePDF(
at: pdfURL, pageCount: 1, creator: "Redline",
annotations: [(page: 0, annotation: makeAnnotation(
.ink, bounds: CGRect(x: 100, y: 100, width: 120, height: 50)
))]
)
let found = try await watcher.scanNow()
#expect(found.contains(where: {
$0.fileURL.resolvingSymlinksInPath().path == pdfURL.resolvingSymlinksInPath().path && $0.isCommented
}))
let commented = try await ledger.commented()
#expect(commented.contains(where: {
$0.fileURL.resolvingSymlinksInPath().path == pdfURL.resolvingSymlinksInPath().path
}))
}
@Test("Launch regression (characterizes the bug): AirDrop-only launch paths with no reconcile miss an OneDrive-mode return")
func airDropOnlyLaunchPathsWithoutReconcileMissesAMarkedOneDriveReturn() async throws {
let suite = try makeTransportDefaultsSuite()
defer { tearDownTransportSuite(suite) }
let oneDriveFolder = try makeTransportTemporaryDirectory(prefix: "shotdeck-buggy-onedrive")
defer { try? FileManager.default.removeItem(at: oneDriveFolder) }
// A configured AirDrop watch-folder override, isolated to a temp dir NOT the real
// ~/Downloads, which may already hold real marked-up Redline PDFs from actual use
// and would make this test's "found.isEmpty" assertion depend on the state of
// Ben's real Downloads folder instead of the isolated fixture under test.
let staleAirDropFolder = try makeTransportTemporaryDirectory(prefix: "shotdeck-buggy-airdrop-stale")
defer { try? FileManager.default.removeItem(at: staleAirDropFolder) }
let appSupportRoot = try makeTransportTemporaryDirectory(prefix: "shotdeck-buggy-approot")
defer { try? FileManager.default.removeItem(at: appSupportRoot) }
TransportSettings.setTransport(.oneDrive, defaults: suite.defaults)
TransportSettings.setOneDriveFolder(oneDriveFolder, defaults: suite.defaults)
FolderSettings.setWatchFolder(staleAirDropFolder, defaults: suite.defaults)
// The BUG's exact construction: FolderSettings.resolvedAppSupportPaths() ignores
// the persisted transport entirely and always resolves the AirDrop folders.
let buggyPaths = try FolderSettings.resolvedAppSupportPaths(root: appSupportRoot, defaults: suite.defaults)
#expect(buggyPaths.watchFolder.path == staleAirDropFolder.path)
#expect(buggyPaths.watchFolder.path != oneDriveFolder.path)
let ledger = try ReturnLedger(paths: buggyPaths)
let watcher = ReturnWatcher(paths: buggyPaths, ledger: ledger)
// The old bootstrap(): recordUncommented was set, but there was NO
// updateWatchFolder() call before start() to reconcile the folder.
await watcher.setRecordUncommented(false)
let pdfURL = oneDriveFolder.appendingPathComponent("Redline-20260905-100100.pdf")
try makePDF(
at: pdfURL, pageCount: 1, creator: "Redline",
annotations: [(page: 0, annotation: makeAnnotation(
.ink, bounds: CGRect(x: 100, y: 100, width: 120, height: 50)
))]
)
// The watcher is still pointed at the stale (configured-AirDrop) watch folder, so
// scanning it NOT the OneDrive folder the PDF actually landed in finds nothing.
// This is the exact BLOCKER the fix above closes.
let found = try await watcher.scanNow()
#expect(found.isEmpty)
}
@@ -90,6 +90,38 @@ func oneDriveFolderUnavailableErrorDescriptionContainsThePath() throws {
#expect(description.contains(path))
}
@Test
func isWritableDirectoryTrueForAnOrdinaryWritableDirectory() throws {
let dir = try makeTransportTemporaryDirectory(prefix: "shotdeck-writable")
defer { try? FileManager.default.removeItem(at: dir) }
#expect(OneDriveLocator.isWritableDirectory(at: dir))
}
@Test
func isWritableDirectoryFalseForAnExistingButUnwritableDirectory() throws {
let dir = try makeTransportTemporaryDirectory(prefix: "shotdeck-unwritable")
defer {
// Restore perms BEFORE removal an unwritable dir can't otherwise be cleaned up.
try? FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: dir.path)
try? FileManager.default.removeItem(at: dir)
}
#expect(OneDriveLocator.isWritableDirectory(at: dir)) // sanity check before chmod
try FileManager.default.setAttributes([.posixPermissions: 0o500], ofItemAtPath: dir.path)
#expect(!OneDriveLocator.isWritableDirectory(at: dir))
}
@Test
func isWritableDirectoryFalseForAPlainFileAndForANonexistentPath() throws {
let dir = try makeTransportTemporaryDirectory(prefix: "shotdeck-writable-check-parent")
defer { try? FileManager.default.removeItem(at: dir) }
let filePath = dir.appendingPathComponent("plain-file.txt")
FileManager.default.createFile(atPath: filePath.path, contents: Data("x".utf8))
#expect(!OneDriveLocator.isWritableDirectory(at: filePath))
#expect(!OneDriveLocator.isWritableDirectory(at: dir.appendingPathComponent("does-not-exist")))
}
struct TransportDefaultsSuite {
let name: String
let defaults: UserDefaults