Repository navigation
fix(gitsync): restore returning sources and record evaluated roots - #3991
Conversation
(cherry picked from commit 9aa8c417840e832597e1ba5d4b0bd5502f5d7098)
(cherry picked from commit 2b1b41d80d5a60fccc8fba8a50d31b50441f829a)
Test Results (shard 1)508 tests 508 ✅ 42s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
Test Results (shard 3)390 tests 390 ✅ 55s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
Test Results (shard 0)254 tests 254 ✅ 2m 22s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate importer findings, plus documentation and immutability fixes, remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request fixes GitSync behavior when returning to earlier sources and ensures scoped imports record evaluated roots.
Changes:
- Validate manifests, including source roots, before skipping imports.
- Reconcile live drift even when Git reports no diff.
- Add regression coverage and update documentation.
File summaries
| File | Review summary |
|---|---|
test/MeshWeaver.Hosting.Test/ReturningSourceConvergesTest.cs |
Adds regression coverage. Nit (2 votes): use an immutable list. |
src/MeshWeaver.Graph/StaticRepoImporter.cs |
Adds manifest and root-aware decisions. Critical (1 vote): make token comparison version-aware. Moderate (1 vote): apply the non-empty-path filter. Nit (2 votes): use ToImmutableDictionary. |
src/MeshWeaver.GitSync/GitHubSyncService.cs |
Updates reconciliation to evaluate live content despite empty Git diffs. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-returning-import-restores-source.md |
Adds the release note. Nit (3 votes): add required Icon frontmatter. |
src/MeshWeaver.Documentation/Data/Architecture/StaticRepoImport.md |
Documents behavior and validation. Two nits (1 vote each): correct stale references from six cases to seven. |
Review details
Suppressed comments (3)
src/MeshWeaver.Documentation/Data/Architecture/StaticRepoImport.md:285
- The added test matrix contains seven cases (2
ReturningToAnEarlierImportedSourcerows, 1 scoped-root fact, 3ImportAtTheRecordedCommitrows, and 1 live-drift fact), so this validation receipt should not say “all six cases.” Please keep the documented count aligned with the actual suite.
only the GitHub I/O boundary. On unchanged core `f1a945d5`, all six cases fail: B → A → B returns `Skipped` (including
src/MeshWeaver.Documentation/Data/Architecture/StaticRepoImport.md:293
- This second validation summary repeats the same stale count:
ReturningSourceConvergesTesthas seven theory/fact cases, not six. Update the receipt so the committed evidence matches the test matrix.
**Initial local validation:** the six-case regression is red on unchanged core `f1a945d5` and green
src/MeshWeaver.Graph/StaticRepoImporter.cs:1210
- The new comparison does not apply the same
!string.IsNullOrEmpty(n.Path)filter asWriteManifest. A source containing an empty-path node would therefore add a token that can never be persisted in the manifest (or makeToDictionarythrow for a null path), causing every repeat to take the full-import path instead of reaching the skip. Filter the source set exactly as the writer does before grouping.
var sourceTokens = nodes.Append(root)
.GroupBy(n => n.Path, StringComparer.OrdinalIgnoreCase)
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 4)1 908 tests 1 908 ✅ 3m 14s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
Test Results (shard 2)1 273 tests 1 271 ✅ 3m 50s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 6 files 6 suites 5m 44s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
Test Results 17 files 17 suites 16m 49s ⏱️ Results for commit 2b87562. ♻️ This comment has been updated with latest results. |
|
CI run The exact local receipt is |
After source B is replaced by A, returning to B could trust B’s old success marker and leave A’s content live. The importer now compares the requested source, including its root, against the current import manifest before skipping. Explicit reconciliation evaluates live drift even when the recorded Git SHA makes the Git diff empty; scoped imports record the root they actually evaluated.
Existing claims, local-edit conflict protection and governance pruning guards remain in place. Content tokens deliberately exclude the mesh owner’s version clock. An executed version-only 42 → 43 → 42 → 43 control confirms distinct import fingerprints, zero writes for identical content, successful historical skips and an unchanged owner clock. The manifest comparison also ignores empty paths, matching the fingerprint and manifest writer; this review regression failed on the previous head and now passes.
Validation: strict Release builds of Hosting.Test, Graph.Test and Documentation.Test report zero warnings and errors. All 9 returning-source cases, 25 existing conflict/scope/sync controls and 12 documentation integrity cases passed. Historical six-case and later scoped-root red/green receipts, plus the distinct review controls, are preserved in Doc/Architecture/StaticRepoImport. CI run 34548770649 passed on the review-corrected head 2b87562 (19/19 jobs). Its shard-5 TRX confirms all 9 ReturningSourceConvergesTest cases executed and passed; Hosting.Test totals 601/601 passed with zero not executed. Production acceptance remains outstanding.
Based on core main 45306a3 after #3968. This draft does not include the separate workflow admission change in core #3981. The preceding #3985 delivery was reported sealed; the current coordinated merge and release hold remains in force while the release owner settles the active generation.
Mirror-sync: none — no UI catalog keys or localized values change.