Various sync fixes from testing with a real (messy) package - #1223
Open
isc-tleavitt wants to merge 8 commits into
Open
Various sync fixes from testing with a real (messy) package#1223isc-tleavitt wants to merge 8 commits into
isc-tleavitt wants to merge 8 commits into
Conversation
A module with <SourcesRoot>.</SourcesRoot> keeps its sources at the module
root. Resource paths are composed as SourcesRoot_"/"_Directory, so they reach
the sync pipeline as "./cls/Foo.cls", while WalkAndHashDirs and the FileHash
baseline key the same file as "cls/Foo.cls". NormalizePath did not collapse "."
segments, so no changed path ever matched the reverse index: nothing was
dispatched to a processor, SyncCompile was skipped (loadItems = 0), and yet
step 9 still printed "Updated:" and committed the new hashes. Every edit in
such a module was silently dropped, reported as a success, and never retried.
Three changes:
- NormalizePath now drops "." path segments, so both spellings collapse to the
same key. A bare "." (SourcesRoot with no Directory) normalizes to "".
- New CanonicalDocName upper-cases the extension of a document name, and both
producers of document names route through it. The storedPaths fallback in
SyncBuildReverseIndex looked up names from RelPathToDocName ("Foo.Bar.CLS")
in an array keyed by ResolveChildren ("Foo.Bar.cls"), so it could never
recover a path the primary index missed.
- A changed path that routes to no resource is now reported as a warning and
removed from the change set before CommitChanges, so it is not reported as
"Updated:" and is re-detected on the next sync instead of being lost.
Tests: unit coverage for the new normalization cases and CanonicalDocName, plus
a sync-dot-root integration fixture and TestSyncWithDotSourcesRoot, which
asserts the edit is live in the namespace rather than merely reported — a green
sync report was exactly the symptom. Verified that the new integration test
fails on the pre-fix source with "Edit is compiled and live on the server" and
passes with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
isc-tleavitt
marked this pull request as ready for review
August 18, 2026 12:48
isc-tleavitt
requested review from
isc-cborbonm,
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer and
isc-pbarton
as code owners
August 18, 2026 12:48
Contributor
Author
|
I've verified this fixes the issue for my target package. The fix is clean and well-tested. |
isc-tleavitt
marked this pull request as draft
August 18, 2026 13:03
Contributor
Author
|
Switching back to draft, going to fix #1224 and whatever else I find too. |
isc-tleavitt
marked this pull request as ready for review
August 18, 2026 16:22
Contributor
Author
|
I think this is good now and I'd like to get it merged to better have sync available for experimentation. |
isc-tleavitt
changed the title
isc-tleavitt
marked this pull request as draft
August 18, 2026 17:07
A plain `sync` commits the changed file's hash, so change detection would never look at that file again and an edit to a unit test synced without -test stayed untested indefinitely. Flag those paths instead (FileHash.TestPending, mirroring SyncFailed): sync collects the changed paths owned by a test-phase UnitTest resource, marks them, and says so. ComputeChanges forces marked rows back into the change set only when the sync runs tests, and CommitChanges clears the flag once they have run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Actually intentionally skipping the changelogs here since they're fixes to an unreleased feature. |
isc-dchui
reviewed
Aug 19, 2026
isc-dchui
left a comment
isc-dchui
left a comment
Collaborator
There was a problem hiding this comment.
Good bug catches! Looks good overall but I tend to find LLM comments/documentation to be excessively verbose and off in a particular backwards-looking describing-what-used-to-happen way as opposed to a describing what is currently implemented way, if that makes sense.
I usually just ask Claude to write comments with concision in mind and make it undergo a verbosity check pass.
Drop backwards-looking/negative-space narration, the intra-release issue reference, and the pre-property backward-compatibility notes; state what the code does and why instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Did a verbosity pass over the comments this PR adds, cutting the "here's what would otherwise happen" framing in favor of describing current behavior. Trimmed:
Comment-only — |
Contributor
Author
This was Claude on auto mode... experimenting here, clearly. |
Contributor
Author
|
Interesting - Claude wasn't using AGENTS.md which directs properly here. I'm tacking on a fix for that too. |
Collect the comment/doc conventions raised in review (#1223) into a Comments & Documentation section. Add a CLAUDE.md that imports AGENTS.md so Claude Code picks the guidelines up automatically; AGENTS.md remains the single source of truth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Don't want to throw Claude off on changelogs.
isc-tleavitt
marked this pull request as ready for review
August 20, 2026 12:03
isc-dchui
approved these changes
Aug 20, 2026
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
2 participants
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.
Fixes #1222
Fixes #1224
Fixes unfiled issue where running sync -test on a package with multiple unit test resources would run the synced test once per resource. (Ignoring Package filter on unit test resources.)