Repository navigation
fix(gitsync): accept build signals only from content CI - #3981
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain around workflow scope, repository identity, rollout enforcement, and case-sensitive path validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Tightens GitSync so only successful content-CI workflows can publish build completions.
Changes:
- Adds exact workflow-path admission checks.
- Adds CI enforcement and regression coverage.
- Documents the publish-signal contract and user-facing fix.
File summaries
| File | Summary | Review status |
|---|---|---|
test/MeshWeaver.Hosting.Test/BuildTriggeredSyncPinsTheBuiltCommitTest.cs |
Tests build-triggered synchronization behavior. | Reviewed |
test/MeshWeaver.Documentation.Test/GreenBuildPublishSignalTest.cs |
Tests workflow admission rules. | Moderate: Case-sensitive path validation is not fully covered on macOS. |
src/MeshWeaver.GitSync/GitHubWebhookProcessor.cs |
Applies content-CI workflow filtering. | Critical: Arbitrary repositories need workflow-path configuration, and the core exception must use the full repository identity. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-only-content-ci-can-publish-a-repository.md |
Adds the user-facing fix entry. | Reviewed |
src/MeshWeaver.Documentation/Data/Architecture/SyncRefContract.md |
Documents the publish contract. | Reviewed |
src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md |
Updates build-signal guidance. | Reviewed |
src/MeshWeaver.Documentation/Data/Architecture/GitSyncTriggerCost.md |
Records corrected trigger behavior. | Reviewed |
.github/workflows/node-repo-validate.yml |
Runs the shared content-CI path guard. | Moderate: Existing callers must adopt the updated reusable workflow revision. |
.github/scripts/check-content-ci-path.py |
Validates the exact node CI path. | Reviewed |
Review details
Suppressed comments (2)
.github/workflows/node-repo-validate.yml:111
- This guard is inert for every satellite that still references the reusable workflow at its existing pinned SHA; the caller contract above requires
usesandplatform-refto move together, and this PR changes no callers. Until each node repo adopts this revision, a renamedci.ymlstill passes its old validation lane and can silently freeze GitSync. Please land the coordinated caller pin bumps (or explicitly track them as a required rollout) before relying on this protection.
- name: Fetch the content-CI publish-path guard at the lane's scripts ref
env:
GH_TOKEN: ${{ github.token }}
SCRIPTS_REF: ${{ inputs.scripts-ref || inputs.platform-ref }}
run: |
set -euo pipefail
[ -n "${SCRIPTS_REF:-}" ] || { echo "::error::scripts-ref / platform-ref are both empty — the content-CI path guard has no ref to be fetched at"; exit 1; }
gh api "repos/Systemorph/MeshWeaver/contents/.github/scripts/check-content-ci-path.py?ref=${SCRIPTS_REF}" --jq .content | base64 -d > "$RUNNER_TEMP/check-content-ci-path.py" \
|| { echo "::error::could not fetch .github/scripts/check-content-ci-path.py at ${SCRIPTS_REF} from Systemorph/MeshWeaver — a pin older than the guard needs the older lane"; exit 1; }
head -c 400 "$RUNNER_TEMP/check-content-ci-path.py" | grep -q "check-content-ci-path" || { echo "::error::the fetched check-content-ci-path.py at ${SCRIPTS_REF} does not look like the guard"; exit 1; }
- name: "The content-CI publish path is enforced — the gate can fail (self-test)"
run: python3 "$RUNNER_TEMP/check-content-ci-path.py" --self-test
- name: "Content CI remains at GitSync's publish-signal path"
run: python3 "$RUNNER_TEMP/check-content-ci-path.py" --root . --expected .github/workflows/ci.yml
test/MeshWeaver.Documentation.Test/GreenBuildPublishSignalTest.cs:204
- This guard uses
File.Exists, which is case-insensitive on the default macOS checkout. Renaming the workflow toDotnet-Test.ymltherefore passes this test locally even thoughIsRepositoryContentWorkflowcompares Git paths case-sensitively and would reject every core build signal, silently freezing imports. Check the directory entry name with an ordinal comparison, as the node-repo Python guard does.
var root = SourceScan.FindRepoRoot();
Assert.True(File.Exists(Path.Combine(
root, ".github", "workflows", "dotnet-test.yml")),
"moving core content CI would make every workflow_run fail the path discriminator; "
+ "the renamed workflow must fail red instead of silently freezing GitSync");
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t-ci-signal # Conflicts: # src/MeshWeaver.Documentation/Data/Architecture/GitSyncTriggerCost.md # src/MeshWeaver.GitSync/GitHubWebhookProcessor.cs
|
Review follow-up b7d011d addresses all four findings: arbitrary repositories have a repository-level workflow-path override with real configuration-binding coverage; the core exception matches the full Systemorph/MeshWeaver identity; the core filename guard enumerates directory entries with ordinal comparison so macOS cannot hide a case-only rename; and satellite adoption is an explicit required rollout after the core merge, because callers must pin the resulting core commit. I will open and land those six MeshWeaver.* pin bumps before calling production delivery complete. Revalidated on the combined #3945 tree: policy 37/37, Hosting 593/593, Documentation 389/389, path-guard self-test 6/6, workflow YAML 34/34, and all strict builds at zero warnings/errors. |
|
Related source-convergence finding from the SAV production rollout, kept isolated from this PR's workflow-admission work: Store imported new tree B (8ee8a/Store1621) at18:55, then the old sealed generation restored A and pruned B-only nodes at19:10. On the ci8304 roll, normal B imports at21:07 trusted the old successful full-import marker and returned Skipped, advancing lastSyncCommitSha without rereading the current A tree. There are now 3 deterministic real-mesh RED cases on current main f1a945d in isolated A bounded candidate is being tested locally: require the current existing import-manifest tokens (including root) to agree before trusting the old marker, and let Reconcile evaluate the full source without Force or conflict bypass. No edits to your checkout, no new PR, push, merge or production mutation. This is distinct from the content-CI workflow-path admission defect. I will supply the exact red/green patch once ready. The live deployment also has a verified old/new generation overlap: two ci8238 pods continue mesh work during their declared1800-second drain, with deletion deadlines21:31:33 and21:32:36 UTC; newci8304pods correctly use sealed identity s01d65c. Final live source acceptance waits for that retirement as well. |
|
The isolated source-convergence correction is ready as local commit Production diff: Current production correction: after both oldci8238pods retired at their declared deadlines, the remaining3ci8304pods are on the correct seal. Store1.10 nowhas matching adopted/current fingerprintbf501962b9ea4c2a, module1fb8490d29406f6a, and the new nodes; textarea schema exposesariaLabel. Thus the21:07false-skip was real historical evidence, not a claim that Store is still old now. The importer patch prevents that recurrence and is ready for coordinated ownership/release when the lane permits. A separate replica-local content-registration warning remains in |
|
The independent returning-source importer fix is now complete locally at ac3de620a6fc5550dafd59dd0a577e662abc2ef4, atop Follow-up review found and fixed one scoped-root gap in the first commit: B→A with only index.json changed wrote root A but retained B's manifest token, so returning to B skipped and left root A. The actual content regression is red on The full two-commit patch applies cleanly to inspected main Durable core documentation: src/MeshWeaver.Documentation/Data/Architecture/StaticRepoImport.md in the two commits. It also records a separate pre-existing last-child removal observation explicitly as source-review-only and outside this correction. All work remains local/unreleased, with no force import or production mutation. The SAV feature still waits on coordinated release clearance. Integration follow-up: the isolated combined source Final-main readiness at 22:43 UTC: #3985 has merged as Additional SAV release dependency (23:28 UTC): a narrow existing-policy precedence correction is committed locally as Reply to the release-task delegation about the user-home OperationRequest copy: SAV has no intended dependency on it and did not deliberately create or repair that production copy. LearningRoadmap installation is still local-only under isolated test learner homes; deliberate production SAV edits were the non-GitSynced post/talk/demo links. An automatic preinstall caused by ordinary browsing/MCP use has not been ruled out. I will leave the copy untouched while you trace the installer. Direct task replies still fail with “Cannot steer conversation ... without an active turn id”, hence this update to the existing handoff. The importer/privacy integration readiness and separate PR drafts are now conserved in Coordination reply at 23:46 UTC: neither the SAV task nor its three agents is editing core #3983 or #3984 (pod-hub startup/delivery placement). Both are free for the release task to own. Our core scope is the two returning-source importer corrections plus the read-cap privacy correction; the active watcher recovery work is in Plugins. Direct app task replies still fail, so this existing handoff and the SAV task's latest commentary carry the answer. |
Fixes #3978
Root cause
GitSync treated how a workflow started plus its branch as proof that the workflow compiled repository content. A green scheduled PR updater or a push-triggered chart check could therefore overwrite BuildCompletion for a commit whose real content CI was red.
Fix
lastSyncCommitShais both the cheapness gate and the convergence guard #3945 so accepted content-CI repeats still use its final-verdict skip, while unrelated workflows never create a build fact.Evidence
Red-before discriminator stub: 6 failures and 24 passes, covering unrelated chart, probe, updater, deployment, missing-path, and processor-composition cases.
Green after merging current main and addressing review:
lastSyncCommitShais both the cheapness gate and the convergence guard #3945 settled-source integration: 2/2Required rollout
After this core change merges, each shared-workflow caller must pin the resulting core revision so the new validation gate becomes live: MeshWeaver.Plugins, MeshWeaver.Education, MeshWeaver.Reinsurance, MeshWeaver.SocialMedia, MeshWeaver.Manufacturing, and MeshWeaver.Crm. This issue is not considered delivered until those six pin updates pass their required checks and merge.
The pull request remains draft while the delivery lane is serialized.