fix(sync): sanitize upstream commits one by one - #333
Conversation
As discussed internally with team.
…mplate` tags As discussed internally with team.
Related commit: gorhill@25d4138
* Add set/unset/closed/given cookie values * Closed already added
GitHub rejects GITHUB_TOKEN pushes that modify .github/workflows/* without the `workflows` permission; granting that scope would let a workflow edit leak secrets, so the automated sync push is blocked whenever upstream's commit range touches .github/workflows/*. Add .github/sync_mirror.sh (unifies CI + admin break-glass, supersedes #329): fetch upstream/master + origin/mirror; checkout -B mirror upstream/master; rebase the new (origin/mirror..upstream/master) commits onto origin/mirror with --keep-empty --exec 'git restore --source=HEAD^ --staged --worktree -- .github/ && git commit --amend --no-edit --allow-empty' so each replayed commit carries no .github/ diff. Push --force-with-lease (rebased commits get new SHAs). Code changes flow through unchanged; upstream-only .github/ changes become empty commits (preserved via --keep-empty) so master<-mirror merges no longer hit .github/ conflicts. sync-from-fork.yml: sync job now clones brave/uBlock (which ships the script), adds upstream, and runs sync_mirror.sh with ORIGIN_URL embedding the GITHUB_TOKEN. test-sync-steps.yml: add sync_mirror.sh to trigger paths and a new test-sync-sanitize job that builds temp upstream/origin repos, seeds origin/mirror, makes an upstream commit touching both .github/workflows/w.yml and code.txt, runs the script, and asserts code.txt change present, w.yml unchanged, and the unsanitized upstream commit unreachable from the pushed mirror. Also covers the workflow-only case via --allow-empty. sync_mirror.sh sets a local user.email/user.name fallback only when none is configured so `git commit --amend` works on CI runners without identity, and the sanitize test runs from a temp working clone so the brave/uBlock checkout's real upstream remote is not used. No deletion commit: auditing all five Brave workflows (assign-pr, check-release-merged, main, sync-from-fork, test-sync-steps) — all are enabled. RELEASE.HEAD.md is consumed by main.yml (Brave actively patches it to use softprops/action-gh-release@v2), and ISSUE_TEMPLATE/config.yml gates issue UX. No upstream-only enabled-workflow files exist to delete, so sanitize alone is sufficient. Closes #329.
chore(sync): sanitize .github/ from mirror commits via rebase
Related commit: gorhill@10ae629
Brave can't be told apart from Chrome through the user agent string, so detection uses `navigator.brave` with `navigator.userAgentData.brands` as fallback. Both are synchronous, unlike `navigator.brave.isBrave()` -- the flavor must be settled before filter lists are compiled and cached. Brave still offers uBO in MV2, and a considerable number of Brave users run Brave Shields and uBO at the same time. Filters which are safe on their own can conflict when both blockers apply them, and list maintainers currently have no way to express that: uBlockOrigin/uAssets#34162
Related issue: uBlockOrigin/uBlock-issues#4090
All supported browsers support user stylesheets, and in any case filter list maintainers shouldn't have to worry about whether user stylesheets are supported or not.
"uBlock Origin dev build" is no longer available in the Chrome Web store.
"uBlock Origin dev build" is no longer available in the Chrome Web store.
"uBlock Origin dev build" is no longer available in the Chrome Web store.
Fork Sync: Update from parent repository
80d5721 to
dda2088
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues in the new replay implementation (upstream SHA parsing) and workflow setup (potentially shallow history) that can break syncing or replay the wrong commit range.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reworks the fork mirror sync to deterministically replay upstream history commit-by-commit while sanitizing .github/ so the mirror push is accepted with a GITHUB_TOKEN that lacks workflows permission.
Changes:
- Replace the rebase/squash sync approach with a deterministic per-upstream-commit replay that preserves author metadata and adds an
Upstream: <owner>/<repo>@<sha>trailer. - Update the fork-sync workflow to run the script from the checked-out ref (instead of a separate
git clone). - Expand the sync sanitize test workflow to cover bootstrap replay, empty commit retention, incremental sync, idempotency, self-heal after mirror deletion, and
SANITIZE=0.
File summaries
| File | Description |
|---|---|
| .github/workflows/test-sync-steps.yml | Extends CI scenarios to validate per-commit replay behavior and .github/ sanitization invariants. |
| .github/workflows/sync-from-fork.yml | Simplifies workflow setup by using actions/checkout and running the in-repo sync script. |
| .github/sync_mirror.sh | Implements deterministic per-commit replay with .github/ tree swapping and upstream trailers. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| PREV="$(git rev-parse "refs/remotes/${REMOTE_ORIGIN}/${BRANCH_MIRROR}")" | ||
| UP_BASE="$(git log -1 --format=%B "$PREV" | grep -oE '[0-9a-f]{40}' | tail -1 || true)" | ||
| if [ -n "$UP_BASE" ]; then |
| for C in $(git rev-list --reverse --topo-order --no-merges "${UP_BASE}..${UP}"); do | ||
| git read-tree "$C" | ||
| git rm -rfq --cached --ignore-unmatch .github | ||
| if git cat-file -e "${PREV}:.github" 2>/dev/null; then | ||
| git read-tree --prefix .github/ "${PREV}:.github" | ||
| fi | ||
| TREE="$(git write-tree)" | ||
| AD="$(git log -1 --format=%aI "$C")" | ||
| NEW="$(GIT_AUTHOR_NAME="$(git log -1 --format=%an "$C")" \ | ||
| GIT_AUTHOR_EMAIL="$(git log -1 --format=%ae "$C")" \ | ||
| GIT_AUTHOR_DATE="$AD" \ | ||
| GIT_COMMITTER_DATE="$AD" \ | ||
| git commit-tree "$TREE" -p "$PREV" -m "$(git log -1 --format=%B "$C") | ||
|
|
||
| Upstream: ${UP_SLUG}@${C}")" | ||
| PREV="$NEW" | ||
| COUNT=$((COUNT + 1)) | ||
| done |
dda2088 to
146c680
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The replay currently builds commit messages via shell command substitution, which can alter upstream message content (trailing newline stripping) and undermines the stated deterministic/preservation guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
| AD="$(git log -1 --format=%aI "$C")" | ||
| NEW="$(GIT_AUTHOR_NAME="$(git log -1 --format=%an "$C")" \ | ||
| GIT_AUTHOR_EMAIL="$(git log -1 --format=%ae "$C")" \ | ||
| GIT_AUTHOR_DATE="$AD" \ | ||
| GIT_COMMITTER_DATE="$AD" \ | ||
| git commit-tree "$TREE" -p "$PREV" -m "$(git log -1 --format=%B "$C") | ||
|
|
||
| Upstream: ${UP_SLUG}@${C}")" |
There was a problem hiding this comment.
Fixed in 42124df: the message is now written to a file (git cat-file commit | sed '1,/^$/d' for the raw bytes — log --format=%B appends its own terminator newline, so it would have corrupted them) and passed to commit-tree -F. Verified byte-for-byte: messages ending in blank lines, without a final newline, and with trailing whitespace are preserved verbatim, with exactly one blank line before the Upstream: trailer.
| # Deterministic byte-wise sorting: git ls-tree output order must match `sort` | ||
| # order for the set operations below, independent of runner locale. |
There was a problem hiding this comment.
Fixed in 42124df: comment updated to describe the actual current rationale (locale-independent sed trailer parsing for deterministic replay).
146c680 to
42124df
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated test workflow’s attempt to avoid pipefail is ineffective under GitHub Actions’ default bash settings, and the new UP_SLUG derivation can break trailer parsing for common SSH remote URL formats.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
| # `<owner>/<repo>` used in the `Upstream:` trailer; GitHub auto-links | ||
| # `<owner>/<repo>@<sha>` as a cross-repo commit reference. | ||
| UP_SLUG="${UP_SLUG:-$(printf '%s' "$UPSTREAM_URL" | sed -E 's#\.git$##; s#^[^/]+//[^/]+/##')}" |
| # NOTE: no pipefail here: `producer | grep -q` lets the producer die | ||
| # of SIGPIPE (exit 141) on match, which pipefail would turn into a | ||
| # spurious failure (and a false-pass inside `if` conditions). | ||
| set -eu |
antonok-edm
left a comment
antonok-edm
left a comment
There was a problem hiding this comment.
i'm open to trying it
Replace the rebase-based mirror sync (whose bootstrap pushed raw upstream history and got rejected by the GITHUB_TOKEN workflows permission) with a deterministic per-commit replay rooted at the upstream/master..master merge-base: - every upstream commit becomes exactly one mirror commit: original author, author date and message preserved, plus an 'Upstream: <owner>/<repo>@<sha>' trailer for cross-repo linking - each replayed commit's .github/ equals its parent's, so no pushed commit ever modifies .github/ and the token push is always accepted - .github-only upstream commits become empty commits, keeping the upstream history 1:1 - pinned committer identity/dates make replays byte-for-byte stable: re-runs push nothing, deleting the mirror self-heals by rebuilding the identical chain from the merge-base Point the sync job at the ref the workflow ran on (actions/checkout of github.ref): schedule keeps using the default branch, while workflow_dispatch now runs the dispatched branch's sync_mirror.sh instead of master's. Extend test-sync-sanitize: bootstrap replay, empty-commit retention, incremental append, idempotent re-run, self-heal determinism and SANITIZE=0 break-glass.
42124df to
cb6f9a1
Compare
|
Fork Sync PR #334 came out dirty (conflicts in Update in cb6f9a1: chain now roots at the current master tip and replays only upstream commits missing from it by patch-id ( |
Summary
workflows-permission rejection) and the squash-based approach (reverted in fix(ci): sanitize .github from mirror sync commits #331/Fork Sync: Update from parent repository #332) with a deterministic per-commit replay:Upstream: gorhill/uBlock@<sha>trailer (GitHub cross-repo autolink).github/is identical to its parent's \u2192 no pushed commit ever touches.github/.github-only upstream commits become empty commits (1:1 upstream history)merge-base(upstream/master, master): bootstrap works without mirror; deleted mirror self-heals byte-for-byte; re-runs with no new upstream commits push nothingSANITIZE=0break-glass unchangedTest plan
test-sync-sanitizeextended: bootstrap per-commit replay, empty-commit retention, incremental append, idempotent re-run, self-heal determinism,SANITIZE=0