refactor: move RSS parsing infrastructure to CelestraKit and fix iOS …

[leogdion]

…parser bug

Move SyndiKitParser and related models from CelestraApp to CelestraKit for sharing between iOS app and server-side CLI tool:

• SyndiKitParser.swift → CelestraKit/Services/
• FeedParserError.swift → CelestraKit/Errors/
• ParsedFeed, ParsedAuthor, ParsedSyndicationUpdate → CelestraKit/Models/
• FeedFormat (includes FeedCategory enum) → CelestraKit/Models/

Critical bug fix:

• Fixed conditional compilation in SyndiKitParser (#if !canImport(FoundationNetworking)) to correctly use URLSession on iOS instead of throwing unsupported error
• This resolves the issue where all feed updates were failing with FeedParserError

Updated imports:

• Added 'public import CelestraKit' to 7 files using moved types
• Removed circular import from ParsedFeed

CelestraKit remains pure (no CloudKit/CoreData dependencies) for cross-platform use.

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 0% with 166 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.07%. Comparing base (14bb477) to head (d333cee).

Files with missing lines	Patch %	Lines
Sources/CelestraKit/Services/SyndiKitParser.swift	0.00%	101 Missing ⚠️
...es/CelestraKit/Services/URLSessionHTTPClient.swift	0.00%	18 Missing ⚠️
Sources/CelestraKit/Models/ParsedFeed.swift	0.00%	15 Missing ⚠️
Sources/CelestraKit/Errors/FeedParserError.swift	0.00%	14 Missing ⚠️
Sources/CelestraKit/Models/FeedFormat.swift	0.00%	10 Missing ⚠️
Sources/CelestraKit/Models/ParsedAuthor.swift	0.00%	4 Missing ⚠️
...s/CelestraKit/Models/ParsedSyndicationUpdate.swift	0.00%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (14bb477) and HEAD (d333cee). Click for more details.

HEAD has 7 uploads less than BASE

Flag	BASE (14bb477)	HEAD (d333cee)
spm	2	1
swift-6.2-jammy	2	1
swift-6.2-noble	2	1
swift-6.2-noblenightly	2	1
swift-6.3-noblenightly	2	1
swift-6.3-jammynightly	2	1
swift-6.2-jammynightly	2	1

Additional details and impacted files

@@             Coverage Diff             @@
##           v0.0.1       #8       +/-   ##
===========================================
- Coverage   88.06%   63.07%   -24.99%     
===========================================
  Files          12       19        +7     
  Lines         419      585      +166     
===========================================
  Hits          369      369               
- Misses         50      216      +166     

Flag	Coverage Δ
spm	63.07% <0.00%> (-24.99%)	⬇️
swift-6.2-jammy	63.07% <0.00%> (-24.99%)	⬇️
swift-6.2-jammynightly	63.07% <0.00%> (-24.99%)	⬇️
swift-6.2-noble	63.07% <0.00%> (-24.99%)	⬇️
swift-6.2-noblenightly	63.07% <0.00%> (-24.99%)	⬇️
swift-6.3-jammynightly	63.07% <0.00%> (-24.99%)	⬇️
swift-6.3-noblenightly	63.07% <0.00%> (-24.99%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leogdion]

preconcurrency isn't necessary

[claude]

Pull Request Review - RSS Parsing Infrastructure Move

This PR moves RSS parsing components from CelestraApp to CelestraKit and fixes a critical iOS parser bug. Overall this is a well-executed refactor with a critical bug fix, but there are several issues that should be addressed.

---

✅ Strengths

1. Critical Bug Fix

The conditional compilation fix in SyndiKitParser.swift:25-61 is correct and solves a blocking issue:

#if \!canImport(FoundationNetworking)
  // Apple platforms - URLSession is in Foundation
  private let session: URLSession
  public func parse(url: URL) async throws -> ParsedFeed { ... }
#else
  // Linux - throw unsupported error
  public func parse(url: URL) async throws -> ParsedFeed {
    throw FeedParserError.networkError(...)
  }
#endif

This correctly enables URLSession on iOS/Apple platforms while maintaining cross-platform compatibility.

2. Good Error Handling Design

FeedParserError.swift follows Swift best practices:

• Proper Sendable conformance for concurrency safety
• Associated values for underlying errors
• Clear, descriptive error messages

3. Clean Model Design

The parsed models (ParsedFeed, ParsedAuthor, etc.) are well-designed:

• Proper Sendable, Codable, and Identifiable conformances
• Clear separation from CloudKit models
• Good use of public import Foundation following project conventions

---

🔴 Critical Issues

1. Missing Copyright Headers ❌

All 6 new files are missing proper copyright headers. Every other file in the project uses the full MIT license header. Compare:

Current (new files):

//  Created for Celestra on 2025-08-13.

Expected (existing files):

//  Created by Leo Dion.
//  Copyright © 2025 BrightDigit.
//
//  Permission is hereby granted, free of charge...

Action Required: Add proper MIT license headers to all 6 new files to match the project's licensing standards.

2. Unsafe Sendable Conformance ⚠️

SyndiKitParser.swift:23 uses @unchecked Sendable which bypasses Swift 6's strict concurrency checking:

public final class SyndiKitParser: @unchecked Sendable {
  private let session: URLSession  // URLSession IS Sendable on Apple platforms
  private let synDecoder: SynDecoder  // Unknown if Sendable

Problems:

• URLSession is Sendable on Apple platforms, so @unchecked is unnecessary for it
• SynDecoder from SyndiKit may not be Sendable - need to verify
• This defeats the purpose of the project's strict concurrency settings (-strict-concurrency=complete)

Recommended Fix:

#if \!canImport(FoundationNetworking)
  // Option 1: If SynDecoder is Sendable
  public final class SyndiKitParser: Sendable {
    private let session: URLSession
    private let synDecoder: SynDecoder
  }
  
  // Option 2: If SynDecoder is NOT Sendable, use actor isolation
  public actor SyndiKitParser {
    private let session: URLSession
    private let synDecoder: SynDecoder
  }
#endif

Verify SynDecoder's Sendable conformance and use proper isolation instead of @unchecked.

3. Non-Cryptographic Hash for UUID Generation ⚠️

SyndiKitParser.swift:66-86 uses SHA256 for deterministic UUIDs:

private func uuid(from url: URL) -> UUID {
  let hash = SHA256.hash(data: Data(urlString.utf8))
  // Convert to UUID...
}

Issues:

• SHA256 is overkill for this use case (UUID generation doesn't need cryptographic security)
• Creates unnecessary dependency on CryptoKit/Crypto
• The UUID is converted to String immediately in mapToArticle anyway

Recommended Fix:
Since feedRecordName is used as a String in Article, just use a deterministic string directly:

// In mapToParsedfeed:
let feedID = url.absoluteString  // Or url.absoluteString.hashValue if you need numeric stability
let articles = try feedable.children.map { entryable in
  try mapToArticle(entryable, feedID: feedID)
}

// In mapToArticle:
fileprivate func mapToArticle(_ entryable: any Entryable, feedID: String) throws -> Article {
  // ...
  let feedRecordName = feedID
  // ...
}

This is simpler, faster, and removes the crypto dependency.

---

🟡 Code Quality Issues

4. Silent URL Fallback is Dangerous ⚠️

SyndiKitParser.swift:133-139 creates placeholder URLs for missing article URLs:

if let url = entryable.url {
  urlString = url.absoluteString
} else {
  // Create a placeholder URL using the entry ID if URL is missing
  urlString = "https://example.com/\(entryable.id)"
}

Problems:

• Creates invalid/fake URLs that could propagate through the system
• Users might click on https://example.com/... links and get confused
• Violates the principle of fail-fast for invalid data

Recommended Fix:
Throw an error for missing URLs since Article.url is non-optional and should be valid:

guard let url = entryable.url else {
  throw FeedParserError.invalidArticleURL
}
let urlString = url.absoluteString

This enforces data integrity at the parsing layer.

5. Incorrect Version Compatibility 🔍

Package.swift:65 shows:

.package(url: "https://github.com/brightdigit/SyndiKit.git", from: "0.7.0"),

But CLAUDE.md:30 documents:

- **SyndiKit** (v0.6.1): RSS/Atom feed parsing

Action Required: Update CLAUDE.md to reflect the actual SyndiKit version (0.7.0+) for documentation accuracy.

6. Weak Feed Format Detection 🔍

SyndiKitParser.swift:160-185 uses string matching on type names:

switch String(describing: type(of: feedable)) {
case let name where name.contains("RSS"):
  return .rss
// ...
}

Problems:

• Fragile - breaks if SyndiKit renames types
• Relies on implementation details rather than protocol capabilities
• Not covered by tests

Recommended Fix:
Use type checking or protocol conformance:

switch feedable {
case is RSSFeed:  // If SyndiKit exposes concrete types
  return .rss
case is AtomFeed:
  return .atom
// ...
}

Or add a feedType property to SyndiKit's Feedable protocol.

---

🟢 Minor Issues

7. Missing GUID Validation

SyndiKitParser.swift:145 uses:

let guid = entryable.id.description

No validation that GUID is non-empty. While unlikely to be empty, add a guard for robustness:

let guid = entryable.id.description
guard \!guid.isEmpty else {
  throw FeedParserError.missingRequiredData
}

8. Inconsistent Naming Convention

mapToParsedfeed (line 88) should be mapToParsedFeed (capital F) to follow Swift naming conventions.

9. Missing Author Validation

Multiple empty authors become empty string:

let authorString = entryable.authors.map(\.name).joined(separator: ", ")
// Later: authorString.isEmpty ? nil : authorString

Works correctly but could be more explicit:

let authors = entryable.authors.map(\.name).filter { \!/bin/bash.isEmpty }
let authorString = authors.isEmpty ? nil : authors.joined(separator: ", ")

---

🧪 CRITICAL: Missing Test Coverage ❌

No tests were added for the new parsing infrastructure. Given that:

1. This fixes a critical bug that blocked all feed updates
2. It introduces 6 new files and 371 lines of code
3. The parser has complex platform-specific compilation paths
4. Feed parsing is a core feature

Action Required: Add comprehensive tests covering:

// Suggested test cases:
- SyndiKitParserTests:
  - ✓ Parse valid RSS feed
  - ✓ Parse valid Atom feed  
  - ✓ Parse feed with missing article URLs (should throw)
  - ✓ Parse feed with missing GUIDs (should throw)
  - ✓ Feed format detection for RSS/Atom/JSON/Podcast/YouTube
  - ✓ Author string joining with multiple authors
  - ✓ Deterministic UUID generation from same URL
  - ✓ Platform-specific compilation (#if \!canImport checks)

- ParsedFeedTests:
  - ✓ Codable round-trip
  - ✓ Default values
  
- FeedParserErrorTests:
  - ✓ Error messages
  - ✓ Sendable conformance

Recommendation: Don't merge without at least basic parser tests, especially for the bug fix path.

---

📋 Summary

Category	Status	Count
Critical Issues	🔴	3
Code Quality Issues	🟡	4
Minor Issues	🟢	3
Missing Tests	❌	1

Before Merge:

1. MUST FIX:

   • Add MIT license headers to all 6 files
   • Add basic parser tests (at minimum, test the iOS URLSession path)
   • Fix @unchecked Sendable or document why it's necessary

2. SHOULD FIX:

   • Throw error for missing URLs instead of creating fake ones
   • Use simpler deterministic ID instead of SHA256 UUIDs
   • Update CLAUDE.md SyndiKit version

3. NICE TO HAVE:

   • Improve feed format detection
   • Add GUID validation
   • Fix mapToParsedfeed → mapToParsedFeed

Estimated Effort:

• Critical fixes: ~2-3 hours (headers + basic tests + Sendable fix)
• Should fix items: ~1-2 hours
• Nice to have: ~1 hour

Great work on the bug fix and refactor! The architecture is solid, just needs polish before merge. 🚀

[claude]

Pull Request Review: RSS Parsing Infrastructure Migration

Overview

This PR successfully moves RSS parsing infrastructure from CelestraApp to CelestraKit, enabling code sharing between the iOS app and server-side CLI tool. The critical bug fix for iOS URLSession support is a major improvement.

✅ Strengths

1. Critical Bug Fix (SyndiKitParser.swift:47-84)

The conditional compilation fix is excellent - this correctly resolves the issue where iOS was incorrectly throwing unsupported errors.

2. Architecture & Code Quality

• Simplification: Replacing 332 lines of HTTP/cache abstraction with URLSession built-in caching is a smart architectural decision
• Sendable Compliance: All types properly conform to Sendable for Swift 6 strict concurrency
• Public Imports: Correct use of public import throughout (matching project Swift language configuration)
• Cross-Platform Design: Clean separation between Apple platforms and Linux/server environments

3. Dependencies

The addition of swift-crypto provides proper cross-platform hashing support, essential for the deterministic UUID generation.

⚠️ Issues & Recommendations

1. CRITICAL: Concurrency Safety Violation (SyndiKitParser.swift:45)

Using @unchecked Sendable is unsafe here. URLSession IS Sendable on modern Swift/iOS, so this should be public final class SyndiKitParser: Sendable without @unchecked. The @unchecked suppresses compiler safety checks and could hide real concurrency bugs.

Recommendation: Remove @unchecked and let the compiler verify safety.

2. Security Issue: Placeholder URL Generation (SyndiKitParser.swift:162)

Creating placeholder URLs like https://example.com/\(entryable.id) violates the Article model contract, creates broken links, and could cause CloudKit uniqueness issues. Better to fail fast with guard let url = entryable.url else { throw FeedParserError.invalidArticleURL } rather than silently creating invalid data.

3. Type Inference Fragility (SyndiKitParser.swift:199-208)

Using String(describing: type(of: feedable)) to detect feed format is fragile and could break with SyndiKit library updates. Consider using Swift type checking instead (if feedable is SyndiKit.RSSFeed) or accepting .unknown as acceptable.

4. Error Handling: Lost Error Context (FeedParserError.swift:37-39)

Associated errors with any Error break Sendable conformance in Swift 6 strict concurrency mode. Consider storing error messages as String instead, or use a custom SendableError wrapper.

5. Memory Management: URLSession Lifecycle (SyndiKitParser.swift:62)

URLSession instances should be explicitly invalidated when done to free resources. Add a deinit or cleanup() method that calls session.invalidateAndCancel().

6. Missing Tests

No test coverage added for UUID generation, feed/article mapping, or format detection. Please add unit tests for these critical components.

7. Documentation Gaps

Public API methods like parse(url:) are missing doc comments explaining parameters, return values, and throws behavior.

🎯 Minor Observations

1. Copyright years updated 2025 → 2026 (verify if intentional)
2. Hardcoded cache sizes (20MB/100MB) might need configurability for server use
3. Good use of @preconcurrency import for SyndiKit
4. SyndiKit version bump 0.6.1 → 0.7.0 should be tested

🏁 Summary

Overall Assessment: Good refactoring with critical bug fix, but needs security and concurrency improvements before merge.

Must Fix Before Merge:

1. Remove @unchecked Sendable (use proper Sendable)
2. Fail on missing article URLs (do not create placeholders)
3. Fix FeedParserError to be properly Sendable

Should Fix:
4. Add test coverage
5. Improve feed format detection reliability
6. Add URLSession cleanup
7. Add documentation comments

The architecture is solid and the bug fix is essential, but the concurrency and security issues need addressing to meet the project Swift 6 strict concurrency standards.

---

Generated with Claude Code

[claude]

Pull Request Review: Move RSS Parsing Infrastructure to CelestraKit

Overview

This PR successfully moves RSS parsing infrastructure from CelestraApp to CelestraKit, enabling code sharing between the iOS app and server-side CLI tool. The refactoring shows good architectural decisions and proper cross-platform support.

---

✅ Strengths

1. Clean Architecture & Separation of Concerns

• HTTPClientProtocol abstraction (Sources/CelestraKit/Services/HTTPClientProtocol.swift:33) provides excellent testability and dependency injection
• SyndiKitParser properly delegates HTTP concerns to the protocol, maintaining single responsibility
• Protocol-oriented design makes testing straightforward (mock HTTP clients can be injected)

2. Excellent Cross-Platform Support

• Proper conditional compilation for FoundationNetworking on Linux (SyndiKitParser.swift:39-41)
• Swift Crypto fallback for non-Apple platforms (SyndiKitParser.swift:33-37)
• @preconcurrency import SyndiKit (line 31) properly handles SyndiKit non-Sendable conformance

3. Strong Concurrency Safety

• All new types properly marked as Sendable (ParsedFeed, ParsedAuthor, ParsedSyndicationUpdate, FeedFormat, FeedParserError)
• Added SwiftLint custom rule to prevent @unchecked Sendable (.swiftlint.yml:134-138) - excellent proactive enforcement
• HTTPClientProtocol correctly marked as Sendable (HTTPClientProtocol.swift:33)

4. Good Error Handling

• FeedParserError provides clear, localized error messages with underlying error propagation
• HTTPClientError properly categorizes HTTP vs response errors (URLSessionHTTPClient.swift:37-40)

5. Critical Bug Fix

• PR description mentions fixing conditional compilation for URLSession on iOS - this resolves the reported feed update failures

---

⚠️ Issues & Concerns

1. Critical: Non-Sendable Error Types (Security/Concurrency)

Location: FeedParserError.swift:37-39

Issue: Error is not Sendable in Swift, but FeedParserError is marked Sendable. This violates strict concurrency checking and could lead to data races when errors are thrown across concurrency boundaries.

Impact: Could cause undefined behavior when errors cross actor boundaries and violates the project commitment to strict concurrency safety.

Recommended Fix: Store error messages as String instead of Error, or create a SendableErrorWrapper type.

2. Bug: Placeholder URLs Are Invalid (Data Integrity)

Location: SyndiKitParser.swift:138-144

Issue: Creates fake URLs using example.com when article URL is missing. This is a reserved domain that should never be used in production URLs.

Impact: Users might try to open these fake URLs, and it violates Article model expectations.

Recommended Fix: Either throw FeedParserError.invalidArticleURL or use a custom URL scheme like feed://feedID/guid

3. Fragile: Feed Format Detection Using String Matching (Maintenance)

Location: SyndiKitParser.swift:180-188

Issue: Relies on internal type name strings from SyndiKit which will break silently if SyndiKit renames internal types.

Recommended: Use type checking instead of string matching.

4. Performance: Inefficient Podcast Detection (Medium Priority)

Location: SyndiKitParser.swift:172-174

Issue: Iterates through ALL articles just to check if ANY have media.

Recommended Fix: Use first or lazy.contains for short-circuit evaluation.

5. Data Loss: Default Fallback to Date() for Missing Published Dates (Data Integrity)

Location: SyndiKitParser.swift:161

Issue: Uses current date when published date is missing, making articles appear newer than they are.

Recommended: Either make publishedDate optional or fail gracefully.

---

📊 Test Coverage Concerns

Missing Test Coverage: The PR does not include any test files. For infrastructure this critical, tests should cover edge cases for SyndiKitParser, URLSessionHTTPClient, and FeedParserError.

---

🎯 Summary

Overall Assessment: This is a solid refactoring that properly moves shared code to CelestraKit with good cross-platform support. The architecture is clean and the bug fix for iOS URLSession usage is critical.

Must Fix Before Merging:

1. ✋ Sendable conformance for FeedParserError (Issue 1) - violates strict concurrency
2. ✋ Remove fake example.com URLs (Issue 2) - data integrity concern

Should Fix:
3. Feed format detection fragility (Issue 3)
4. Podcast detection performance (Issue 4)
5. Default published date fallback (Issue 5)

Recommended:
6. Add test coverage
7. Add documentation about URLSession thread-safety

Great work on the refactoring! The architecture improvements are solid, and the bug fix is important. Just need to address the concurrency safety issue before merging.