docs: review on every push, now that a push costs a delta - #87
Conversation
The recommended workflow analyzed a pull request once, when it became reviewable, because every run re-derived the whole thing and a run per push was not worth it. A run now continues from the pull request's previous analysis and covers only the commits pushed since, so the reason for leaving synchronize out is gone. It also matters for a second reason: a pushed commit is the only thing that builds that reusable analysis. GitHub gives comment-triggered runs a read-only cache, so a repository reviewed only through /codeboarding starts from the base every time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeBoarding reviewStatus: 0 changed components See the full change in CodeBoarding. graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
n_Visual_Rendering_Engine -- "Returns rendering metadata and diagram artifacts" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "Provides annotated diff model for visualization" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Triggers structural comparison via CLI" --> n_Structural_Diff_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;
|
![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: 3c7ddd1a09
ℹ️ 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".
Recommending a run on every push without a concurrency group invites two races. Two pushes in quick succession analyze concurrently, so both restore the same older analysis and neither continues from the other, which is the saving the per-push recommendation is made for. Worse, every pull_request run writes the same sticky comment header, so whichever run finishes last wins, and that can be the one for the older commit: the review then describes a head that has already been superseded. The example now queues one review per pull request, as this repository's own workflow has done, and says what the alternative costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ac9399a7d
ℹ️ 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".
| concurrency: | ||
| group: codeboarding-${{ github.event.pull_request.number || github.event.issue.number }} | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
When one review is running and a synchronize run is pending, any subsequent issue_comment event—even an ordinary or unauthorized comment—enters this workflow-level group before the job-level if filters it. GitHub documents that an existing pending workflow in the same group is canceled and replaced by the newly queued one, so the comment can cancel the pending push review and then skip the review job itself, leaving the sticky diagram at the older SHA indefinitely. Avoid placing filtered-out comment events in the pull-request review group, for example by applying concurrency at the eligible job or separating their group.
Useful? React with 👍 / 👎.
|
|
||
| `synchronize` re-runs the review on every push to the branch. Each of those runs covers only the commits pushed since the previous one, so a push costs a fraction of a first analysis — and a pushed commit is the only thing that builds the reusable analysis, since GitHub gives comment-triggered runs a read-only cache. Drop `synchronize` from the list if you would rather spend one analysis per pull request than one per push. | ||
|
|
||
| Keep the `concurrency` block if you keep `synchronize`: it is what makes a push continue from the push before it, and what stops a slower run for an older commit from overwriting the review comment for a newer one. Set `cancel-in-progress: true` instead to abandon a superseded run rather than queue it, which costs less when branches are pushed to rapidly, at the price of no analysis for the commits in between. |
There was a problem hiding this comment.
Even when every queued event is a valid synchronize, cancel-in-progress: false only prevents simultaneous execution; GitHub explicitly states that ordering is not guaranteed for workflows in a concurrency group. Two close pushes can therefore have the newer review execute first and the older one afterward, and because automatic runs select the same codeboarding-review identifier in scripts/action/guard.sh:122-123, that older run can still overwrite the newer diagram. The action needs a head-SHA freshness check before publishing (or another ordering mechanism) before this block can be documented as preventing stale overwrites.
Useful? React with 👍 / 👎.
| on: | ||
| pull_request: | ||
| types: [opened, reopened, ready_for_review] | ||
| types: [opened, reopened, ready_for_review, synchronize] |
There was a problem hiding this comment.
For repositories following the protected-branch setup at README.md:192-210, every baseline update is force-pushed to the machine-owned codeboarding/sync PR, so adding synchronize here runs the paid review job after every such update even though that PR contains only generated baseline state. The repository's own review workflow explicitly excludes this branch at .github/workflows/codeboarding.yml:35-41 because its diagram is noise; the public example should copy that same-repository github.head_ref exclusion so the two recommended modes do not repeatedly review each other's generated output.
Useful? React with 👍 / 👎.
1 participant
Adds
synchronizeto the recommendedpull_requesttrigger, and the per-pull-requestconcurrencyblock that makes it safe.The trigger list deliberately left
synchronizeout, because every run used to re-derive the whole analysis and one run per push was not worth paying for. That reason is gone: a run now continues from the pull request's previous analysis and covers only the commits pushed since it.There is a second reason, which #86 documents. A pushed commit is the only thing that builds that reusable analysis — GitHub gives comment-triggered runs a read-only cache token, so a repository reviewed exclusively through
/codeboardingre-derives the head from the base every time.Measured on a real 17-file pull request in CodeBoarding-webview: the head analysis was 4m26s of the 6.5-minute run. With a chain in place, a run after two more commits covers those two commits, not all seventeen.
Why the concurrency block is part of this, not a nicety
Recommending per-push runs without serialization invites two races, both raised in review:
pull_requestrun writes the same sticky header,codeboarding-review(guard.sh:122-123) — onlyissue_commentruns get a per-run header. So whichever run finishes last overwrites the comment, and that can be the run for the older commit. The pull request then shows a diagram for a superseded head.The example now queues one review per pull request, matching what this repository's own workflow has always done, and the README states what
cancel-in-progress: truetrades instead.Worth considering separately
The concurrency block protects consumers who copy the example. Anyone who adds
synchronizeto an existing workflow without it still hits the sticky-comment race. An action-side guard — refusing to overwrite a comment that already describes a newer commit — would make that safe regardless of workflow shape. Out of scope here.Docs only — no behaviour change in the action. This repository's own workflow is left as it is, since that is a spending decision rather than a recommendation.
🤖 Generated with Claude Code