feat(sync): pull_request delivery strategy for protected branches - #60
feat(sync): pull_request delivery strategy for protected branches#60Svilen-Stefanov wants to merge 3 commits into
Conversation
Adds `sync_strategy: pull_request` for repositories whose default branch rejects direct pushes. Instead of committing the architecture baseline straight to `target_branch`, it commits to a machine-owned branch (`sync_pr_branch`, default `codeboarding/sync`) and opens/force-updates a single rolling PR into `target_branch`. Default stays `push`, so existing consumers are unchanged. - action.yml: new `sync_strategy`/`sync_pr_branch`/`sync_pr_title` inputs and `sync_pr_url`/`sync_pr_number` outputs. The commit step rebuilds the rolling branch on the LIVE `target_branch` tip and force-pushes, then find-or-creates exactly one open PR (all gh calls fail open on an under-scoped token). - Incremental sync seeds only from the baseline committed on `target_branch`, never the unmerged sync branch, so no untrusted pkl is unpickled on the runner; merge the rolling PR on a cadence to keep the baseline warm. - Loop guard in pull_request mode is `paths-ignore` + the existing nothing-to-commit gate (a merged baseline PR touches only generated files). - Fix a pre-existing `paths-ignore` bug that ignored `.codeboarding/health/**` and thus swallowed the user-authored `.healthignore`/`health_config.json`; list only the generated `health_report.json`, and add `fingerprint.json`/`static_analysis.pkl`/`static_analysis.sha`. - Exclude `codeboarding/sync` from auto-review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Architecture review · no architectural changesgraph LR
n_Analysis_Orchestrator["Analysis Orchestrator"]
n_Visual_Diffing_Rendering_Engine["Visual Diffing #38; Rendering Engine"]
n_Documentation_CTA_Generator["Documentation #38; CTA Generator"]
n_Telemetry_Feedback_Provider["Telemetry #38; Feedback Provider"]
n_Analysis_Orchestrator -- "Orchestrates structural comparison and visualiz…" --> n_Visual_Diffing_Rendering_Engine
n_Analysis_Orchestrator -- "Provides health metrics for navigation context" --> n_Documentation_CTA_Generator
n_Analysis_Orchestrator -- "Reports execution status and quota metrics" --> n_Telemetry_Feedback_Provider
n_Visual_Diffing_Rendering_Engine -- "Supplies visual context for component documenta…" --> n_Documentation_CTA_Generator
n_Documentation_CTA_Generator -- "Resolves symbol-to-file mappings for deep-linki…" --> n_Visual_Diffing_Rendering_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
Colors indicate component changes compared to target branch Download the PR analysis artifacts from this workflow artifact. Explore this PR’s architecture in your browser or VS Code. codeboarding-action · run 30300579929 |
![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: ab0dc493e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| echo "::warning::Could not fetch ${TARGET_BRANCH} to rebuild ${SYNC_PR_BRANCH}; skipping this run. The next repository change can regenerate." | ||
| exit 0 | ||
| fi | ||
| git checkout -B "$SYNC_PR_BRANCH" FETCH_HEAD |
There was a problem hiding this comment.
When an existing rolling PR contains a generated delta and a later source push makes the regenerated artifacts match target_branch again (for example, the source change is reverted before the baseline PR is merged), this resets only the local branch. The unchanged/timestamp-only gates at lines 1566-1575 then exit before the force-push and PR handling, leaving the remote PR proposing the obsolete baseline, which can subsequently be merged. The no-op path must reset the remote sync branch and close or otherwise reconcile the stale PR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I spend some time to understand what are the implications:
The revert bug occurs only with:
mode: sync
sync_strategy: pull_requestand only while the rolling baseline PR is still open.
main: M1 ── M2 ── M3
sync branch / PR: └── S2
M1ismainwith the current committed architecture baseline.- Someone pushes
M2tomain, adding/changing source code. - Sync runs, sees that the generated baseline differs from
main, and createsS2oncodeboarding/sync. The open PR isS2 → main. - Before that PR is merged, someone pushes
M3tomainthat reverts the source change fromM2. - Sync runs again. The generated baseline now matches the baseline already on
main, so it decides there is no work to commit.
At step 5, the action should also neutralize the existing S2 → main PR—but currently it exits early instead. The remote codeboarding/sync branch and its PR still contain S2, whose generated files describe the reverted change.
So the stale PR remains mergeable even though the change it describes is no longer in main.
This does not happen with sync_strategy: push, because there is no separate rolling branch/PR to leave stale.
There was a problem hiding this comment.
My suggestion after prompting is:
Make each generated sync PR immediately self-explanatory:
[CodeBoarding Sync] 8f31c2a: Revert "Add payment component"
And use the body for the stable context:
Generated from source revision: `8f31c2a` — Revert "Add payment component"
Target branch: `main`
This branch is machine-managed. Its generated baseline is replaced on each sync.For me the following is optional:
For the skip: the action should still be triggered by the revert push to main.
It cannot know the baseline is up to date until it runs the analysis and compares the newly generated output with what is on main. Then it can take the existing “nothing changed” skip path.
Allegedly we should detect if the content is the same as before (via sha comparison) and produce it instantly, or at least I think so?
That distinction matters:
Revert pushed to main
→ action runs
→ regenerated output equals main’s committed baseline
→ skip creating a new baseline commit
→ close the prior sync PR, if one exists
If the workflow did not trigger for the revert, it would never discover that the previously open sync PR had become stale.
A paths-ignore rule is still useful for the opposite case: do not rerun sync merely because the generated CodeBoarding baseline files themselves were merged into main.
| permissions: | ||
| contents: write # push the sync branch | ||
| pull-requests: write # open/update the rolling PR |
There was a problem hiding this comment.
When this snippet is applied to the preceding default free-tier setup, its job-level permissions block replaces the workflow-level permissions but omits id-token: write. With the shown action invocation and no llm_api_key, the action cannot obtain its required OIDC credential and fails before reaching the new PR delivery path. Include id-token: write here or explicitly configure an API key in the example.
Useful? React with 👍 / 👎.
| if [ "$SYNC_STRATEGY" = "pull_request" ]; then | ||
| [ -n "$SYNC_PR_BRANCH" ] || { echo "::error::sync_pr_branch is empty."; exit 1; } |
There was a problem hiding this comment.
If a caller accidentally sets sync_pr_branch equal to target_branch, this validation accepts it and the later git push --force targets the base branch itself instead of creating a PR. On a branch that rejects force pushes the feature silently produces no PR, while a token allowed to force-push can overwrite concurrent target-branch changes. Require the two branch inputs to be different before analysis starts.
Useful? React with 👍 / 👎.
| (github.event_name == 'pull_request' && github.event.action != 'closed' && github.event.pull_request.draft == false && | ||
| github.head_ref != 'codeboarding/sync') || |
There was a problem hiding this comment.
For a fork PR whose contributor branch happens to be named codeboarding/sync, github.head_ref has this same bare value, so the review job is skipped even though the PR is unrelated to the machine-owned baseline branch. Include the head repository identity in this condition so only this repository's actual sync PR loses automatic architecture review.
Useful? React with 👍 / 👎.
| # live target_branch tip each run, so the push is never a fast-forward. | ||
| # Serialized by the workflow's concurrency group, so there is no | ||
| # force-with-lease race to guard. Fail-open, like the push strategy below. | ||
| if ! git push --force "$AUTH_URL" "HEAD:refs/heads/${SYNC_PR_BRANCH}"; then |
There was a problem hiding this comment.
When a consumer enables this strategy without the optional workflow-level concurrency block, two sync runs can both fetch the base, finish out of order, and this unconditional force-push lets the older analysis overwrite the newer rolling branch. A composite action cannot assume the caller supplied the concurrency group referenced in the comment, so delivery should use a remote-aware lease/retry mechanism or make serialized execution an explicit required configuration.
Useful? React with 👍 / 👎.
|
|
||
| - **Merge the rolling PR on a cadence.** The baseline reaches `target_branch` only when this PR is merged. That is what keeps review-mode diffs fast (review reads the baseline from the PR's base branch) *and* keeps incremental sync warm: each sync run seeds its incremental analysis from the baseline committed on `target_branch`, and re-detects **every** change since that last-merged baseline (change detection is whole-tree content-based, so no commits in between are ever missed — just larger diffs the longer you wait to merge). The action never seeds from the unmerged sync branch, so a poisoned or half-written baseline on that branch can never influence an analysis. | ||
| - **Merging the PR does not re-trigger a full analysis.** A merge of a baseline-only PR changes only generated files, which the `paths-ignore` above already excludes — so the workflow never starts. Keep that list complete and generated-only: your own `.codeboardingignore` / `.healthignore` / `health_config.json` are deliberately left out so editing analysis scope still regenerates. (If you drop a generated path from the list, the merge triggers one run that then no-ops at the "nothing to commit" step — a single wasted run, never a loop.) | ||
| - **Exclude the sync branch from your other PR workflows.** The rolling PR would otherwise trigger your review workflow, tests, and lint on a baseline-only diff. Add a head-branch guard to each — e.g. `if: github.head_ref != 'codeboarding/sync'` on the review job, and `branches-ignore: [codeboarding/sync]` (or the same `if:`) on test/lint workflows. |
There was a problem hiding this comment.
In a pull_request trigger, branches-ignore filters the PR's target branch, not its head branch. Following this recommendation with branches-ignore: [codeboarding/sync] therefore still runs test and lint workflows for the rolling PR into main, while unexpectedly filtering PRs whose base is named codeboarding/sync. Recommend a job-level github.head_ref guard instead.
Useful? React with 👍 / 👎.
| # push_token must carry pull-requests: write. The default github.token | ||
| # does when the job grants it (above). A GitHub App token also works and | ||
| # attributes the PR to your app — but note a github.token-opened PR does | ||
| # NOT trigger other workflows, while an App/PAT-opened PR does. |
There was a problem hiding this comment.
Granting pull-requests: write to github.token is not sufficient when the repository or organization has disabled the separate “Allow GitHub Actions to create and approve pull requests” setting. In that common configuration, the shown default setup pushes the sync branch but every gh pr create fails open, so no rolling PR is delivered. Document the required repository setting here or make an App/PAT token the prescribed fallback rather than implying that the job grant alone is enough.
Useful? React with 👍 / 👎.
Resolves the review findings on #60. - Reconcile a stale rolling PR on no-op: when target_branch already carries the current baseline (e.g. a source change was reverted before its baseline PR merged), delete the remote sync branch to auto-close the now-obsolete PR instead of exiting early and leaving it mergeable. Covers both the nothing-to-commit and timestamp-only exits. - Validate sync_pr_branch != target_branch (else the force-push would target the base branch itself). - Force-push with --force-with-lease against the pre-run branch tip so a concurrent run (when the caller omits the required concurrency group) is not silently clobbered; document the concurrency block as required. - Record the analyzed source revision (short SHA + subject) in the rolling PR body, refreshed every run via gh pr edit. - Scope the review-workflow sync-branch exclusion to same-repo PRs so a fork branch named codeboarding/sync is still reviewed. - Docs: add id-token: write to the protected-branch example; correct the sync-branch exclusion guidance (head_ref if:, not branches-ignore); document the "Allow GitHub Actions to create and approve pull requests" setting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f911a8a51f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| else | ||
| force_opt="--force" | ||
| fi |
There was a problem hiding this comment.
When two first-time pull_request sync runs overlap before sync_pr_branch exists (for example, the caller missed the required concurrency block or a manual dispatch overlaps a push), both see an empty SYNC_TIP and this path falls back to plain --force, so the older run can still overwrite the newer branch update. Fresh evidence in this revision is that the no-existing-branch path remains an unconditional force push; Git supports --force-with-lease=<ref>: for the “must not already exist” case, so the initial create can be protected too.
Useful? React with 👍 / 👎.
| permissions: | ||
| contents: write # push the sync branch | ||
| pull-requests: write # open/update the rolling PR | ||
| id-token: write # free hosted tier (omit if you set llm_api_key/license_key) |
There was a problem hiding this comment.
In the protected-branch example, users who set license_key are told they can omit id-token: write, but the action's license mode still mints a GitHub OIDC token before contacting the hosted proxy and exits with an error when that permission is missing. This breaks the documented setup specifically for licensed protected-branch users; only BYO llm_api_key should be called out as safe to omit OIDC.
Useful? React with 👍 / 👎.
| # force-push against it (see the push below). Empty when the branch does | ||
| # not exist yet (first run) — then the push plain-creates it. | ||
| SYNC_TIP="$(git ls-remote "$AUTH_URL" "refs/heads/${SYNC_PR_BRANCH}" 2>/dev/null | awk '{print $1; exit}')" | ||
| git checkout -B "$SYNC_PR_BRANCH" FETCH_HEAD |
There was a problem hiding this comment.
When a newer push lands on target_branch while an older sync run is still analyzing, this resets the rolling PR branch to the newer FETCH_HEAD but then commits artifacts generated for the older ANALYZED_SHA. The queued newer run may eventually correct the PR, but until then the rolling PR is mergeable and can land a baseline that does not describe its parent commit; if FETCH_HEAD differs from the analyzed SHA, this run should fail open and let the queued run publish.
Useful? React with 👍 / 👎.
| # Ensure ONE open PR from the sync branch into target_branch. Every gh call | ||
| # fails open: an under-scoped token (no pull-requests: write) or an API | ||
| # error degrades to "branch pushed, no PR" with a warning, never a red job. | ||
| pr_number="$(gh pr list --repo "$REPOSITORY" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json number --jq '.[0].number // empty' 2>/dev/null || true)" |
There was a problem hiding this comment.
If an unrelated fork PR into the same base uses the same head branch name as sync_pr_branch, this gh pr list --head lookup can select that fork PR because the GitHub CLI manual documents --head as a branch-name filter and says "<owner>:<branch>" syntax is not supported (https://cli.github.com/manual/gh_pr_list). Fresh evidence here is that the PR-management path now uses the same bare branch-name filter, so it can edit the contributor PR body and report success without creating the repo-owned rolling baseline PR.
Useful? React with 👍 / 👎.
| reconcile_stale_sync_pr() { | ||
| [ "$SYNC_STRATEGY" = "pull_request" ] || return 0 | ||
| git ls-remote --exit-code --heads "$AUTH_URL" "$SYNC_PR_BRANCH" >/dev/null 2>&1 || return 0 | ||
| if git push "$AUTH_URL" --delete "refs/heads/${SYNC_PR_BRANCH}" >/dev/null 2>&1; then |
There was a problem hiding this comment.
When the required concurrency block is missing, an older run that produces no local diff can reach this no-op reconciliation after a newer run has already pushed a valid sync_pr_branch; because the delete path only checks that the branch exists, it removes that newer branch and closes the rolling PR instead of leaving the newer baseline mergeable. The stale-PR cleanup needs to verify that the remote branch/PR is actually obsolete for this run before deleting it.
Useful? React with 👍 / 👎.
… cases Addresses a second round of review on #60. - Defer a run when target_branch advanced PAST the analyzed commit (git merge-base --is-ancestor), so the rolling PR's baseline always describes its base; the queued run for the newer commit publishes. The ancestor gate avoids silently deferring forever when target_branch is deliberately decoupled from the triggering ref. - Lease the branch push: force-with-lease against the tip observed at run start, and a plain (non-force) create when the branch does not exist, so a concurrent run (when the required concurrency group is omitted) is never silently clobbered. Guard the empty-array expansion so the first-run push does not abort under `set -u` on bash < 4.4 (fail-open contract preserved). - Reconcile only deletes the sync branch as observed at run start (current == SYNC_TIP), so an older no-op run cannot wipe a newer run's freshly pushed baseline / PR. - Scope PR lookups to isCrossRepository == false so a fork PR that collides on the head branch name is never edited or mistaken for the rolling PR. - Record the analyzed source revision in the PR body, refreshed each run. - Docs: id-token: write is omittable only for llm_api_key; a license still uses the hosted proxy and keeps OIDC (fixes the pre-existing L116 claim too). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What
Adds a second sync delivery strategy so
mode: syncworks on repositories whose default branch is protected against direct pushes. You pick it with one input:sync_strategypush(default, unchanged)target_branchdirectly.pull_request(new)sync_pr_branch, defaultcodeboarding/sync) and opens/updates one rolling PR intotarget_branch.Default is
push, so existing consumers are completely unchanged.How
pull_requestmode workstarget_branchtip + one baseline commit and force-pushes it, then find-or-creates a single open PR (idempotent; allghcalls fail open, so an under-scoped token degrades to "branch pushed, no PR" rather than a red job). Force-pushing onto livetarget_branchkeeps the diff clean and conflict-free even after earlier baselines have merged.target_branchonly when the PR is merged. That is what keeps review-mode diffs fast (review reads the baseline from the PR's base) and keeps incremental sync warm.target_branch, never from the unmerged sync branch — so an untrustedstatic_analysis.pklis never unpickled on a runner that holds write creds + the LLM key. Change detection is whole-tree content-based, so a run always re-detects every change since the last merged baseline (no in-between commit is missed, just larger diffs the longer you wait to merge).Concurrency: does the last commit win?
Yes — guaranteed by the sync workflow's concurrency block (
cancel-in-progress: false, static group), which runs sync strictly one at a time. A later-triggered run can't start until the earlier one finishes, so there's no finish-order inversion; the newest commit's run is always the last to execute and wins.pull_requestmode is actually cleaner here thanpushmode, because it force-pushes (wholesale replace) instead of rebasing.Loop guard (simplified to Layer 1)
Sync writes generated files back, which is itself a push/merge that could re-trigger sync. Prevented by:
paths-ignoreon the workflow trigger — a merged baseline PR touches only generated files, so the workflow never even starts.The list is kept generated-files-only on purpose: your own
.codeboardingignore/.healthignore/health_config.jsonare deliberately not ignored, so editing analysis scope still regenerates.Also in this PR
paths-ignorebug:.codeboarding/health/**was swallowing the user-authored.healthignoreandhealth_config.json, so a health-scope edit never regenerated. Now lists only the generatedhealth_report.json, and adds the previously-missingfingerprint.json/static_analysis.pkl/static_analysis.sha.codeboarding/syncfrom the auto-review workflow.COMMIT_STRATEGY.md: consumer setup for protected branches (pull-requests: write, token scope,paths-ignore, excluding the sync branch from other PR workflows), new inputs/outputs.Notes for review
push(itsmainisn't protected), so the new PR path isn't exercised by our CI — it's covered byactionlint+shellcheck, and should get one real run in a protected-branch repo before we lean on it.main; unrelated version-string differences are left to the0.13.3dependency PR.🤖 Generated with Claude Code