From f69f94ef731d9aa0e25bf36a5fb4e8c30e96591f Mon Sep 17 00:00:00 2001 From: csjones <637026+csjones@users.noreply.github.com> Date: Tue, 21 Apr 2026 16:10:49 -0700 Subject: [PATCH] feat: add tag prefix filtering to prevent spurious updates from historical tags - Added nonNumericPrefix() to extract leading non-digit prefix from tag names - Implemented latestTag(from:matchingPrefixOf:) to filter candidates by prefix equality - Updated update command to scope tag selection to current tag's naming scheme - Enhanced error messages to suggest manual config update when prefix changes - Added comprehensive test coverage for prefix extraction and filtering logic --- .../SubtreeLib/Commands/UpdateCommand.swift | 39 +++-- .../SubtreeLib/Utilities/GitOperations.swift | 51 ++++++- .../GitOperationsTagFilterTests.swift | 142 ++++++++++++++++++ 3 files changed, 218 insertions(+), 14 deletions(-) create mode 100644 Tests/SubtreeLibTests/Utilities/GitOperationsTagFilterTests.swift diff --git a/Sources/SubtreeLib/Commands/UpdateCommand.swift b/Sources/SubtreeLib/Commands/UpdateCommand.swift index d45bda9..4bffccb 100644 --- a/Sources/SubtreeLib/Commands/UpdateCommand.swift +++ b/Sources/SubtreeLib/Commands/UpdateCommand.swift @@ -233,12 +233,16 @@ public struct UpdateCommand: AsyncParsableCommand { newTag = nil newBranch = explicitRef } - } else if entry.tag != nil { - // Configured with a tag - auto-detect latest tag + } else if let currentTag = entry.tag { + // Configured with a tag - auto-detect latest tag in the same naming scheme do { let remoteTags = try await GitOperations.lsRemoteTags(remote: entry.remote) - guard let latestTag = remoteTags.first else { - print("❌ No tags found on remote") + guard let latestTag = GitOperations.latestTag( + from: remoteTags, + matchingPrefixOf: currentTag + ) else { + let prefix = GitOperations.nonNumericPrefix(of: currentTag) + print("❌ No tags matching prefix '\(prefix)' found on remote — upstream may have changed its tag scheme; update `tag:` in subtree.yaml manually") Foundation.exit(1) } targetRef = latestTag.tag @@ -435,11 +439,15 @@ public struct UpdateCommand: AsyncParsableCommand { var newTag: String? = entry.tag let newBranch: String? = entry.branch - if entry.tag != nil { - // Configured with a tag - auto-detect latest tag + if let currentTag = entry.tag { + // Configured with a tag - auto-detect latest tag in the same naming scheme let remoteTags = try await GitOperations.lsRemoteTags(remote: entry.remote) - guard let latestTag = remoteTags.first else { - throw NSError(domain: "UpdateError", code: 1, userInfo: [NSLocalizedDescriptionKey: "No tags found on remote"]) + guard let latestTag = GitOperations.latestTag( + from: remoteTags, + matchingPrefixOf: currentTag + ) else { + let prefix = GitOperations.nonNumericPrefix(of: currentTag) + throw NSError(domain: "UpdateError", code: 1, userInfo: [NSLocalizedDescriptionKey: "No tags matching prefix '\(prefix)' found on remote — upstream may have changed its tag scheme; update `tag:` in subtree.yaml manually"]) } targetRef = latestTag.tag targetCommit = latestTag.commit @@ -569,10 +577,15 @@ public struct UpdateCommand: AsyncParsableCommand { for entry in entriesToCheck { do { - if entry.tag != nil { - // TAG-BASED: Use lsRemoteTags to find latest tag + if let currentTag = entry.tag { + // TAG-BASED: Use lsRemoteTags to find latest tag in the same naming scheme let remoteTags = try await GitOperations.lsRemoteTags(remote: entry.remote) - guard let latestTag = remoteTags.first else { + guard let latestTag = GitOperations.latestTag( + from: remoteTags, + matchingPrefixOf: currentTag + ) else { + let prefix = GitOperations.nonNumericPrefix(of: currentTag) + let errorMessage = "No tags matching prefix '\(prefix)' found on remote — upstream may have changed its tag scheme; update `tag:` in subtree.yaml manually" let reportEntry = ReportEntry( name: entry.name, status: .error, @@ -581,11 +594,11 @@ public struct UpdateCommand: AsyncParsableCommand { currentCommit: nil, branch: nil, remote: entry.remote, - error: "No tags found on remote" + error: errorMessage ) reportEntries.append(reportEntry) if !asJSON { - print("⚠️ \(entry.name): no tags found on remote") + print("⚠️ \(entry.name): \(errorMessage)") } continue } diff --git a/Sources/SubtreeLib/Utilities/GitOperations.swift b/Sources/SubtreeLib/Utilities/GitOperations.swift index 989555d..131d21a 100644 --- a/Sources/SubtreeLib/Utilities/GitOperations.swift +++ b/Sources/SubtreeLib/Utilities/GitOperations.swift @@ -335,7 +335,56 @@ public enum GitOperations { compareSemver(lhs.tag, rhs.tag) == .orderedDescending } } - + + /// Extracts the leading non-numeric prefix of a tag name. + /// + /// Used to scope candidate tags to the same naming scheme as the + /// currently-configured tag when selecting the "latest" tag from a + /// heterogeneous remote. Some upstreams (e.g., openssl/openssl) mix + /// release tags (`openssl-3.6.2`) with historical labels (`rsaref`, + /// `SSLeay_0_9_0`) that would otherwise sort ahead of real releases + /// under pure string comparison. + /// + /// Only ASCII digits (`0`–`9`) terminate the prefix, to avoid + /// surprises from non-ASCII digit-like characters in tag names. + /// + /// Examples: + /// - `"openssl-3.6.2"` → `"openssl-"` + /// - `"tor-0.4.8.21"` → `"tor-"` + /// - `"v1.2.3"` → `"v"` + /// - `"1.2.3"` → `""` + /// - `"rsaref"` → `"rsaref"` + /// - `"OpenSSL_1_1_1w"` → `"OpenSSL_"` + /// - `"release-2.1.12-stable"` → `"release-"` + static func nonNumericPrefix(of tag: String) -> String { + var prefix = "" + for ch in tag { + if ("0"..."9").contains(ch) { break } + prefix.append(ch) + } + return prefix + } + + /// Returns the newest tag from `tags` whose non-numeric prefix matches + /// that of `configuredTag`, or `nil` if none match. + /// + /// `tags` is expected to be sorted latest-first (as returned by + /// ``lsRemoteTags(remote:)``). Matching is strict equality on the + /// extracted prefix (not `hasPrefix`), so a pure-numeric configured + /// tag (`1.2.3`, prefix `""`) will not accidentally match `rsaref` + /// (prefix `"rsaref"`). + /// + /// Returning `nil` on an empty filtered set is intentional: it lets + /// callers surface a clear error on upstream renames rather than + /// silently picking an unrelated tag. + public static func latestTag( + from tags: [(tag: String, commit: String)], + matchingPrefixOf configuredTag: String + ) -> (tag: String, commit: String)? { + let targetPrefix = nonNumericPrefix(of: configuredTag) + return tags.first { nonNumericPrefix(of: $0.tag) == targetPrefix } + } + /// Compare two version strings using semver-like comparison /// Handles formats: "1.2.3", "v1.2.3", "1.2.3-beta", etc. private static func compareSemver(_ lhs: String, _ rhs: String) -> ComparisonResult { diff --git a/Tests/SubtreeLibTests/Utilities/GitOperationsTagFilterTests.swift b/Tests/SubtreeLibTests/Utilities/GitOperationsTagFilterTests.swift new file mode 100644 index 0000000..f6d2002 --- /dev/null +++ b/Tests/SubtreeLibTests/Utilities/GitOperationsTagFilterTests.swift @@ -0,0 +1,142 @@ +import Testing +import Foundation +@testable import SubtreeLib + +/// Tests for tag-prefix filtering used when selecting the "latest" remote tag. +/// +/// Background: `GitOperations.lsRemoteTags` returns tags sorted by a +/// semver-ish comparator that falls back to plain string compare for +/// non-numeric parts. Repos with historical/label tags (e.g., OpenSSL's +/// `rsaref`, `SSLeay_*`) therefore sort ahead of the actual releases +/// (`openssl-3.6.2`), causing spurious update PRs. These tests lock in +/// prefix-aware filtering that scopes candidates to the same naming +/// scheme as the currently-configured tag. +@Suite("GitOperations Tag Filter Tests") +struct GitOperationsTagFilterTests { + + // MARK: - nonNumericPrefix + + @Test("nonNumericPrefix extracts the leading non-digit prefix of a tag") + func nonNumericPrefixBasic() { + #expect(GitOperations.nonNumericPrefix(of: "openssl-3.6.2") == "openssl-") + #expect(GitOperations.nonNumericPrefix(of: "tor-0.4.8.21") == "tor-") + #expect(GitOperations.nonNumericPrefix(of: "v1.2.3") == "v") + #expect(GitOperations.nonNumericPrefix(of: "V1.2.3") == "V") + #expect(GitOperations.nonNumericPrefix(of: "1.2.3") == "") + #expect(GitOperations.nonNumericPrefix(of: "") == "") + } + + @Test("nonNumericPrefix returns full tag when no digit is present") + func nonNumericPrefixNoDigits() { + #expect(GitOperations.nonNumericPrefix(of: "rsaref") == "rsaref") + #expect(GitOperations.nonNumericPrefix(of: "main") == "main") + #expect(GitOperations.nonNumericPrefix(of: "stable") == "stable") + } + + @Test("nonNumericPrefix handles mixed-case and underscore separators") + func nonNumericPrefixMixedFormats() { + #expect(GitOperations.nonNumericPrefix(of: "OpenSSL_1_1_1w") == "OpenSSL_") + #expect(GitOperations.nonNumericPrefix(of: "release-2.1.12-stable") == "release-") + #expect(GitOperations.nonNumericPrefix(of: "SSLeay_0_9_0") == "SSLeay_") + } + + @Test("nonNumericPrefix treats only ASCII 0-9 as digits") + func nonNumericPrefixOnlyASCIIDigits() { + // Ensure we don't accidentally match non-ASCII digit-like characters. + #expect(GitOperations.nonNumericPrefix(of: "v\u{0660}1.2.3") == "v\u{0660}") // Arabic-Indic digit 0 + } + + // MARK: - latestTag(from:matchingPrefixOf:) + + @Test("latestTag picks the first tag sharing the configured tag's prefix") + func latestTagPrefersMatchingPrefix() { + // Pre-sorted as lsRemoteTags would return (latest-first under compareSemver). + let tags: [(tag: String, commit: String)] = [ + ("rsaref", "aaa"), // would sort first under old logic ('r' > 'o') + ("openssl-3.7.0-beta1", "bbb"), + ("openssl-3.6.2", "ccc"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "openssl-3.6.2") + #expect(result?.tag == "openssl-3.7.0-beta1") + #expect(result?.commit == "bbb") + } + + @Test("latestTag regression: OpenSSL tag set does not select rsaref") + func latestTagOpenSSLRegression() { + // Reproduces the real-world tag set from openssl/openssl that caused + // the spurious "update to rsaref" PR in swift-openssl. + // Pre-sorted by compareSemver (simulating lsRemoteTags output). + let tags: [(tag: String, commit: String)] = [ + ("rsaref", "000"), // 'r' > 'o' → would sort first + ("openssl-3.7.0-beta1", "001"), + ("openssl-3.6.2", "002"), + ("openssl-3.6.1", "003"), + ("openssl-3.5.0", "004"), + ("OpenSSL_1_1_1w", "005"), // 'O' < 'o' + ("SSLeay_0_9_0", "006"), + ("BEN_FIPS_TEST_6", "007"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "openssl-3.6.2") + #expect(result?.tag == "openssl-3.7.0-beta1") + #expect(result?.tag != "rsaref") + } + + @Test("latestTag returns nil when no tag matches the configured prefix") + func latestTagReturnsNilOnPrefixRebrand() { + // Simulates an upstream rename: user has foo-* configured but remote + // only publishes bar-* now. Surfacing nil lets the caller raise a + // clear error instead of silently picking an unrelated tag. + let tags: [(tag: String, commit: String)] = [ + ("bar-2.0.0", "aaa"), + ("bar-1.0.0", "bbb"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "foo-1.0.0") + #expect(result == nil) + } + + @Test("latestTag returns nil on empty tag list") + func latestTagReturnsNilOnEmptyList() { + let tags: [(tag: String, commit: String)] = [] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "openssl-3.6.2") + #expect(result == nil) + } + + @Test("latestTag preserves tor-style prefix isolation") + func latestTagTorStyle() { + let tags: [(tag: String, commit: String)] = [ + ("tor-0.4.9.0", "new"), + ("tor-0.4.8.21", "cur"), + ("tor-0.4.8.20", "old"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "tor-0.4.8.21") + #expect(result?.tag == "tor-0.4.9.0") + } + + @Test("latestTag uses strict prefix equality, not hasPrefix") + func latestTagStrictPrefixEquality() { + // Configured tag has empty non-numeric prefix (pure-numeric scheme). + // Tags like "rsaref" have full-string non-numeric prefix and must NOT + // match just because "rsaref".hasPrefix("") is true. + let tags: [(tag: String, commit: String)] = [ + ("rsaref", "junk"), + ("2.0.0", "new"), + ("1.2.3", "cur"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "1.2.3") + #expect(result?.tag == "2.0.0") + } + + @Test("latestTag distinguishes case-sensitive prefixes") + func latestTagCaseSensitivePrefix() { + // openssl/openssl historically mixed `OpenSSL_*` and `openssl-*` + // schemes. A user who configured the lowercase scheme must not be + // migrated to the uppercase one automatically. + let tags: [(tag: String, commit: String)] = [ + ("OpenSSL_3_9_9", "upper"), + ("openssl-3.7.0", "lower-new"), + ("openssl-3.6.2", "lower-cur"), + ] + let result = GitOperations.latestTag(from: tags, matchingPrefixOf: "openssl-3.6.2") + #expect(result?.tag == "openssl-3.7.0") + } +}