release: prepare v1.2.0#75
Merged
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces ChangesWorktree Snapshot and Library Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c612eb931e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+185
to
187
| if let originURL = self.repository?.originURL, !originURL.isEmpty { | ||
| self.id = originURL | ||
| self.identitySource = .gitOrigin |
There was a problem hiding this comment.
WorktreeSnapshot.id is set to originURL whenever Git metadata exists, but Library.threads(inWorktreeID:) and worktreeGroups key off that ID, so two different checkouts/worktrees of the same repository collapse into one logical “worktree.” In that scenario, callers cannot target a single worktree (they always get all threads for that origin), which defeats the new worktree-specific APIs and can mix branch-specific UI/state across separate directories.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ROADMAP.md (1)
192-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate ordered-list numbering in current priority list.
Line 201 repeats
10.after Line 193 already uses10.. Renumbering avoids ambiguous references in plaintext diffs and maintainer discussions.✏️ Suggested renumbering
-10. Flesh out archive-aware retention and eviction beyond the current list-driven +11. Flesh out archive-aware retention and eviction beyond the current list-driven archive-state drift correction. -11. Add any sharper binary-discovery diagnostics we want alongside the +12. Add any sharper binary-discovery diagnostics we want alongside the current-reviewed compatibility window before a broader compatibility release. -12. Revisit whether a convenience `run(...)` API is earned only after the +13. Revisit whether a convenience `run(...)` API is earned only after the lower-level lifecycle has more production mileage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ROADMAP.md` around lines 192 - 206, The ordered list has a duplicate "10." entry; update the second "10." (the item starting "Flesh out archive-aware retention and eviction...") to "11." and increment the subsequent list numbers accordingly (change the existing "11." "Add any sharper..." to "12." and the "12." "Revisit whether a convenience `run(...)` API..." to "13.") so the sequence is unique and unambiguous while preserving the exact item text.Sources/SwiftASB/Public/CodexWorkspace.swift (1)
183-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize empty/whitespace Git facts before computing worktree identity.
If Codex sends
""/whitespace in Git fields,hasFactscan become true while identity still falls back inconsistently. Treating blank strings asnilkeepshasFacts,hasRepositoryFacts, and identity derivation aligned.🧩 Suggested fix
public struct RepositoryInfo: Sendable, Equatable { @@ public init( originURL: String? = nil, branch: String? = nil, sha: String? = nil ) { - self.originURL = originURL - self.branch = branch - self.sha = sha + self.originURL = Self.normalizedFact(originURL) + self.branch = Self.normalizedFact(branch) + self.sha = Self.normalizedFact(sha) } @@ internal var normalized: Self? { isEmpty ? nil : self } + + private static func normalizedFact(_ value: String?) -> String? { + guard let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines), + !trimmed.isEmpty else { + return nil + } + return trimmed + } }Also applies to: 243-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SwiftASB/Public/CodexWorkspace.swift` around lines 183 - 199, The repository normalization must treat empty or whitespace-only Git fields as nil so hasFacts/hasRepositoryFacts and identity derivation stay consistent: update the normalization logic used when assigning self.repository (and any other places like the block around lines 243-249) so that repository?.normalized trims string fields and converts ""/whitespace-only values to nil before you evaluate repository?.hasFacts, compute id/identitySource, or call Self.displayName(forGitOriginURL:); ensure the same nil-normalization is applied wherever Repository.normalized is used so hasRepositoryFacts accurately reflects presence of real Git data.
🧹 Nitpick comments (1)
Tests/SwiftASBTests/Public/CodexAppServerLibraryTests.swift (1)
251-267: ⚡ Quick winAdd direct checks for
threads(in:)andthreads(inWorktreeID:)to complete API coverage.The new test already validates
threads(inRepositoryOriginURL:); adding direct assertions for the other two new selectors would better lock in parity across the full public worktree query surface.♻️ Suggested test additions
`#expect`(library.threads(inRepositoryOriginURL: "https://github.com/gaelic-ghost/SwiftASB.git").map(\.id) == [ "thread-active", ]) + let repoWorktree = try `#require`( + library.worktreeGroups.first(where: { $0.id == "https://github.com/gaelic-ghost/SwiftASB.git" })?.worktree + ) + `#expect`(library.threads(in: repoWorktree).map(\.id) == ["thread-active"]) + `#expect`(library.threads(inWorktreeID: repoWorktree.id).map(\.id) == ["thread-active"]) `#expect`(library.threads( inRepositoryOriginURL: "https://github.com/gaelic-ghost/SwiftASB.git", includeArchived: true ).map(\.id) == [ "thread-active", "thread-archived", ]) + `#expect`(library.threads(in: repoWorktree, includeArchived: true).map(\.id) == [ + "thread-active", + "thread-archived", + ]) + `#expect`(library.threads(inWorktreeID: repoWorktree.id, includeArchived: true).map(\.id) == [ + "thread-active", + "thread-archived", + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/SwiftASBTests/Public/CodexAppServerLibraryTests.swift` around lines 251 - 267, Add direct assertions calling library.threads(in:) and library.threads(inWorktreeID:) to mirror the existing repository-origin tests: call library.threads(in: <Repository> or appropriate param) and library.threads(inWorktreeID: "<worktree-id>") and assert their .map(\.id) returns ["thread-active"] for default and ["thread-active","thread-archived"] when includeArchived: true; reference the library variable and the methods threads(in:) and threads(inWorktreeID:) so the test covers both selectors alongside the existing threads(inRepositoryOriginURL:) checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ROADMAP.md`:
- Around line 192-206: The ordered list has a duplicate "10." entry; update the
second "10." (the item starting "Flesh out archive-aware retention and
eviction...") to "11." and increment the subsequent list numbers accordingly
(change the existing "11." "Add any sharper..." to "12." and the "12." "Revisit
whether a convenience `run(...)` API..." to "13.") so the sequence is unique and
unambiguous while preserving the exact item text.
In `@Sources/SwiftASB/Public/CodexWorkspace.swift`:
- Around line 183-199: The repository normalization must treat empty or
whitespace-only Git fields as nil so hasFacts/hasRepositoryFacts and identity
derivation stay consistent: update the normalization logic used when assigning
self.repository (and any other places like the block around lines 243-249) so
that repository?.normalized trims string fields and converts ""/whitespace-only
values to nil before you evaluate repository?.hasFacts, compute
id/identitySource, or call Self.displayName(forGitOriginURL:); ensure the same
nil-normalization is applied wherever Repository.normalized is used so
hasRepositoryFacts accurately reflects presence of real Git data.
---
Nitpick comments:
In `@Tests/SwiftASBTests/Public/CodexAppServerLibraryTests.swift`:
- Around line 251-267: Add direct assertions calling library.threads(in:) and
library.threads(inWorktreeID:) to mirror the existing repository-origin tests:
call library.threads(in: <Repository> or appropriate param) and
library.threads(inWorktreeID: "<worktree-id>") and assert their .map(\.id)
returns ["thread-active"] for default and ["thread-active","thread-archived"]
when includeArchived: true; reference the library variable and the methods
threads(in:) and threads(inWorktreeID:) so the test covers both selectors
alongside the existing threads(inRepositoryOriginURL:) checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57e26f59-fa2c-4922-ba01-9ea5f0dc980e
📒 Files selected for processing (10)
README.mdROADMAP.mdSources/SwiftASB/Public/CodexAppServer+Library.swiftSources/SwiftASB/Public/CodexAppServer+ThreadLifecycle.swiftSources/SwiftASB/Public/CodexWorkspace.swiftSources/SwiftASB/SwiftASB.docc/SwiftUIObservableCompanions.mdTests/SwiftASBTests/Public/CodexAppServerLibraryTests.swiftTests/SwiftASBTests/Public/CodexWorkspaceTests.swiftdocs/maintainers/v1-public-api-audit.mddocs/maintainers/v1-public-api-symbol-inventory.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release
workspace/git-factsmainupdates behind pull request review and CIv1.2.0was created locally before this PR so the reviewed release candidate is preserved exactlyReview Loop
Before merge,
scripts/repo-maintenance/release.shwatches CI and stops on review comments unless the maintainer has already addressed or resolved them and reruns with--review-comments-addressed.Summary by CodeRabbit
New Features
Documentation
Tests