--output resolves directory-vs-file four different ways, two of them wrong #37

Activity

1. odrobnik

   added

   status::doing

   on Oct 5, 2026
2. odrobnik commented on Oct 5, 2026

   

   odrobnik

   on Oct 5, 2026

   ContributorAuthor

   More actions

   🧭 Planner finished — implementation plan ready for approval (via Claude Team [drobnik] · 3% used).

   Good — no README changes needed (it only shows an existing-directory example, which is unaffected). Here's the plan.

   ---

   Implementation plan: unify --output directory-vs-file resolution (issue #37)

   Intended behavior

   One rule, used identically by post attachment, post pdf, post fetch --eml/--output, and LocalMessageFile's --part:

   • an existing directory is a directory;
   • a path ending in / is a directory, created if missing;
   • anything else is the file to write, with its parent directory created.

   pathExtension is never consulted. This already is LocalMessageFile.destination(for:named:) (Sources/post/Commands/LocalMessageFile.swift:246-254, added in #36) — the fix is to lift it to a shared location and make the other three sites call it instead of re-deriving the rule.

   Files/components involved

   • Sources/post/Commands/LocalMessageFile.swift — has the correct logic today (lines 235-254), used by extract(_:section:to:) (line 229).
   • Sources/post/Commands/Fetch.swift — isExplicitOutputFile (lines 223-232) and the outURL/isExplicitFile/outputDir plumbing in run() (lines 128-143, 158-163, 191-195).
   • Sources/post/Commands/Attachment.swift — inline pathExtension.count > 0 check (lines 43-47).
   • Sources/post/Commands/PDF.swift — inline pathExtension.lowercased() == "pdf" check (lines 38-58).
   • New shared file, e.g. Sources/post/OutputPath.swift (matches the existing convention of top-level helper files like Int+ByteFormatting.swift, String+FetchHeaders.swift).
   • Tests: Tests/PostCLITests/FetchOutputPathTests.swift, Tests/PostCLITests/LocalPartExtractionTests.swift ("Output path resolution" section, lines 115-156), Tests/PostCLITests/AttachmentCommandTests.swift.

   Steps

   1. Create Sources/post/OutputPath.swift with the rule split into two functions so call sites that need to validate before a filename is known (Fetch's and PDF's "reject a filename with multiple UIDs" check) can do so without duplicating logic:

      enum OutputPath {
          /// Whether `output` names a directory under the shared rule — no filesystem mutation.
          static func namesDirectory(_ output: String) -> Bool {
              let url = URL(fileURLWithPath: output)
              let isExistingDirectory = (try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false
              return isExistingDirectory || output.hasSuffix("/")
          }
      
          /// Resolve `output` to the file to write, creating whatever directory is needed.
          static func destination(for output: String, named filename: String) throws -> URL {
              let url = URL(fileURLWithPath: output)
              let wantsDirectory = namesDirectory(output)
              let directory = wantsDirectory ? url : url.deletingLastPathComponent()
              try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
              return wantsDirectory ? directory.appendingPathComponent(filename) : url
          }
      }

      Carry over the doc comment from LocalMessageFile.swift:235-245 explaining why pathExtension doesn't work.

   2. LocalMessageFile.swift: delete the static destination(for:named:) (lines 246-254); change extract (line 229) to call OutputPath.destination(for: output, named: part.suggestedFilename).

   3. Fetch.swift:

      • Delete isExplicitOutputFile(_:fileManager:) (lines 223-232).
      • Replace the multi-UID guard's predicate with let isExplicitFile = output.map { !OutputPath.namesDirectory($0) } ?? false.
      • Drop the precomputed outputDir (lines 136-143); call OutputPath.destination(for: output, named: "\(uidValue).eml") directly in the eml branch, and OutputPath.destination(for: output, named: "\(message.uid).txt") in the plain-body-to-file branch. This also fixes the extensionless-filename bug for Fetch (today invoice with no extension and no existing path is read as "wants directory" because pathExtension.isEmpty).

   4. Attachment.swift: replace lines 43-47 with let destination = try OutputPath.destination(for: output, named: attachment.filename).

   5. PDF.swift:

      • Replace isExplicitFile (line 39) with let isExplicitFile = !OutputPath.namesDirectory(output), keeping it only for the existing "no filename across multiple UIDs" check (lines 42-44) — that check is a separate rule and survives unchanged.
      • Drop the manual outputDir/createDirectory (lines 46-47); replace the destination computation (line 58) with try OutputPath.destination(for: output, named: result.filename).
      • Net effect: archive.pdf as an existing directory is now correctly treated as a directory; report.pdf.bak is now correctly treated as a file; report (no extension, multiple UIDs) is now correctly rejected by the multi-UID check instead of silently becoming a directory.

   6. Help text: give all four --output option declarations the same accurate wording (LocalMessageFile.swift:137, Fetch.swift:23, Attachment.swift:24, PDF.swift:22) — something like "an existing directory, or a path ending in /, takes a generated filename; anything else is the file to write" — replacing the current "directory or filename" phrasing that doesn't describe the actual rule.

   Tests / verification

   • Consolidate the destination-resolution unit tests onto the shared helper: move the "Output path resolution" tests out of LocalPartExtractionTests.swift (lines 117-156) and the three tests in FetchOutputPathTests.swift into one new Tests/PostCLITests/OutputPathTests.swift, retargeted at OutputPath.namesDirectory/OutputPath.destination. This avoids testing the same logic twice under two names.
   • Add the two regression cases the issue calls out that aren't covered yet:
     • existing directory archive.pdf → treated as directory (covers PDF's old .pdf-extension check).
     • report.pdf.bak → treated as a file (covers PDF's old case-sensitivity/exact-match check).
   • Keep LocalPartExtractionTests.swift focused on listing/extraction; it still exercises the shared helper indirectly through extract.
   • AttachmentCommandTests.swift only tests argument parsing today (no live-client call), so it can't exercise destination end-to-end without a server fixture — same constraint the issue notes for why Address a single part of a local message by section #36 didn't touch these three sites. The shared-helper unit tests are the practical coverage; call this out as a residual gap rather than claiming command-level verification.
   • Run swift test --scratch-path "/Volumes/SSD/SwiftPM/$(basename "$PWD")" after the change.

   Open questions / risks

   • Naming/placement of the shared helper is my suggestion (OutputPath enum in a new top-level file) matching this repo's existing Type+Feature.swift convention — a human reviewer may prefer a different name or an extension on URL instead; flagging rather than deciding unilaterally.
   • Behavior change in PDF: dropping the .pdf-extension-specific check for the multi-UID guard means more inputs will now correctly be rejected or accepted per the shared rule (e.g., --output report with multiple UIDs is now rejected as an explicit filename, where before it silently became a directory). This is the intended fix per the issue but is a visible CLI behavior change worth a release note.
   • No live-IMAP test fixture exists in this repo for Fetch/Attachment/PDF's run(), so the fix is verified at the shared-helper unit-test level, not through the actual command path — same limitation the issue itself describes for why Address a single part of a local message by section #36 didn't touch these three sites.

   Approve this plan by starting an Implementor for the issue.
3. odrobnik

   added

   status::review

   status::doing

   and removed

   status::doing

   status::review

   on Oct 5, 2026
4. odrobnik commented on Oct 8, 2026

   

   odrobnik

   on Oct 8, 2026

   ContributorAuthor

   More actions

   🚀 Shipped (via Claude Team [drobnik] · 6% used) — opened #51

   PR #51 is open with auto-merge and squash: #51

   What changed

   • New shared resolver in Sources/post/OutputPath.swift. It states the rule once: an existing directory is a directory, a trailing slash is a directory created if missing, anything else is the filename to write with its parent created. A separate namesDirectory check touches nothing on disk, so commands can reject a filename before any directory exists.
   • All four sites now call it. LocalMessageFile lost its private copy. Fetch lost its existence-plus-extension check and the precomputed output directory, and an explicit filename now also applies to the text-body export instead of being silently ignored. Attachment and PDF lost their pathExtension checks, so exports.v1 and archive.pdf as existing directories now work, and invoice or report.pdf.bak are written as files. The multi-UID filename rejection in pdf and fetch survives on top of the shared rule.
   • Help text for all four --output options now describes the actual rule. The redundant "(default: .)" that ArgumentParser already appends was dropped.
   • Tests consolidated into Tests/PostCLITests/OutputPathTests.swift, replacing the Fetch-only tests and the resolution section of the local-part tests. Two regression cases from the issue were added: an existing archive.pdf directory, and report.pdf.bak as a file.

   Verification. Built with swift build --build-tests and ran the full suite on the SSD scratch. All 43 tests pass. The rewritten help text was checked by running the built binary. No screenshots, since this is a CLI change with no screen.

   Residual gap. As the issue and plan note, fetch, attachment and pdf are only reachable through a live IMAP server, so their command paths are covered by the shared-helper unit tests rather than end to end.

   One note for the release: post pdf --output report with multiple UIDs is now rejected as a filename where it used to silently become a directory. That is the intended behaviour but is a visible change.
5. odrobnik

   added

   status::shipped

   and removed

   status::doing

   on Oct 8, 2026
6. odrobnik

   closed this as completedin #51on Oct 8, 2026